search

Project60 / org.project60.banking

CiviCRM banking extension
18 stars 35 forks source link

SmartyV3 compatibility (on CiviCRM 5.69.5 + PHP 8.1) #419

Closed VangelisP closed 2 months ago

VangelisP commented 5 months ago

Hi all.

I've tried both MRs reported here:

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 commented 5 months ago

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 commented 5 months ago

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:

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 commented 5 months ago

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

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

The draft PR is located here

VangelisP commented 5 months ago

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 commented 5 months ago

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 https://github.com/civicrm/civicrm-core/commit/b6a02f8db405ea0f7355ad57a9dc6e939ecc2ba9.

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 2 months ago

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 https://github.com/Project60/org.project60.banking/pull/420.

VangelisP commented 2 months ago

Fine by me!