SmartyV3 compatibility (on CiviCRM 5.69.5 + PHP 8.1)

[VangelisP]

Hi all.

I've tried both MRs reported here:

• Update Civix-generated code to Civix version 23.02.1 #418 (from @jensschuppe )
• and CiviCRM 5.69 Compatibility #412 (from @jaapjansma )

but even though I tried both, I do still have some issues. While visiting the /civicrm/banking/payments I get:

SmartyException: "undefined extension class 'Smarty_Internal_Runtime_Get_Plugin_Filepath'"

plus the 'Bank accounts' tab does not render. I get to get a similar error on the backend:

[warning] Attributes passed to CRM_Core_Form::add() are not an array. Caller: CRM_Banking_Form_AccountsTab::buildQuickForm

and then at the same time: "message" => "undefined extension class 'Smarty_Internal_Method_Trigger_Error'"

The frontend ui appears empty on that tab.

Site: CiviCRM 5.69.5 + PHP 8.1 + SmartyV3

[VangelisP]

One more issue that I've found (based on this post) is that in theory, some functions have been renamed.

on banking/CRM/Banking/Helpers/Smarty.php -> line 85 -> $oldVars = $this->smarty->get_template_vars(); should become: $oldVars = $this->smarty->getTemplateVars();

Similarly on banking/CRM/Banking/Page/Review.php -> line 316 -> old value: $vars = $this->get_template_vars(); -> $vars = $this->getTemplateVars();

[VangelisP]

After further investigation, I did find the culprit for the /civicrm/banking/payments issue, as Demerit correctly said in this post, the tpl file(s) need reviewing and replacing of the |dateformat declaration as such:

• file: /templates/CRM/Banking/Page/Payments.tpl
• Line 120:
  • <td class="" nowrap>{$field.date|date_format:"%d-%m-%Y"}</td> should become
  • <td class="" nowrap>{$field.date|crmDate}</td>
• Line 177:
• <td nowrap class="crm-contact-activity-activity_type">{$field.date|date_format:"%d-%m-%Y"}</td> should become
• <td nowrap class="crm-contact-activity-activity_type">{$field.date|crmDate"}</td>

Now, regarding the bank accounts tab, debugging the error, it seems that there's an API call that throws this error because, I suppose, doesn't exist: https://github.com/Project60/org.project60.banking/blob/master/templates/CRM/Banking/Form/AccountsTab.tpl#L17

Removing line 17 completely does render the tab properly, which means we got 2 options here:

1. Do the check if this extension exists on the PHP equivalent page and pass a variable if this BIC extension is being installed to the tpl only.
2. Remove completely this line and all references to it.

Thoughts?

[VangelisP]

I've created a branch in my own fork and tried to sort things out.

• I've first applied the branch civixUpgrade via PR Update Civix-generated code to Civix version 23.02.1 #418
• Then added the necessary modifications that I've mentioned on my posts above. Those modifications do the following:
  • Moved the function that detects if the extension BIC is being enabled into the PHP file, now passing only a true/false variable if it's being enabled or not
  • Changed the date format to crmDate

I've tried it in both smarty V3 and smarty V2 and had no issues so far.

The draft PR is located here

[VangelisP]

On my comment above I've wrongly assumed that I needed to switch this function:

Similarly on banking/CRM/Banking/Page/Review.php -> line 316 -> old value: $vars = $this->get_template_vars(); -> $vars = $this->getTemplateVars();

I shouldn't, since this is already being taken care by CiviCRM Core itself .
Luckily, I did not push that into my commit(s), I'm just stating this here so that nobody else will make the same mistake.

[jensschuppe]

this is already being taken care by CiviCRM Core itself

Well, CRM_Core_Form::get_template_vars() is deprecated and the correct replacement in fact is CRM_Core_Form::getTemplateVars() which was added in 5.70.0, see civicrm/civicrm-core@b6a02f8.

So we should either add a version switch or call $this->_template->getTemplateVars() as that's Smarty's method that has "always" been around … I guess the cleanest would be overriding getTemplateVars() in CRM_Banking_Page_Review and calling either parent::getTemplateVars() or parent::get_template_vars() from there. With that we only have to remove the overriding method once we drop support support for CiviCRM < 5.70.0 without checking everywhere in the code.

[VangelisP]

You have a point there. I would go for this approach along with a conditional check on the civicrm version (?). I don't think that it's so heavy to pull the version and do a conditional check and use the proper function, no ?

We could also play it safer and instead of checking the civicrm version, check if the 2 constants: CIVICRM_SMARTY_AUTOLOAD_PATH or CIVICRM_SMARTY3_AUTOLOAD_PATH are being defined.
That's a lighter approach ..

Thoughts?

[jensschuppe]

What about, in the overridden getTemplateVars() of the form/page controller (or in a utility class), checking for method_exists($this, 'getTemplateVars'), and else calling get_template_vars()? This way the IDE will be happy, too.

[VangelisP]

Just tested this conditional check on both CiviCRM 5.69.x (which I am currently working on) and also on the latest tag 5.72.1:

      // Check if this method exists already
      if (method_exists($this, 'getTemplateVars')) {
        $vars = $this->getTemplateVars();
      }
      else {
        $vars = $this->get_template_vars();
      }

For 5.69.x condition that fires up is the 2nd, while on 5.72.1, it's the first ($this->getTemplateVars()) so I think we're being covered, what do you think @jensschuppe ?

[pminf]

With version 1.1.0 CiviBanking seems to be compatible with Smarty 3. I guess we can close this issue, as it has been fixed with #420.

[VangelisP]

Fine by me!