Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Wip] Smarty3 - branch for testing #27565

Closed

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 22, 2023

Overview

[Wip] Smarty3 - branch for testing

This adds code that makes it possible to

  1. enabled Smarty3 as an extension
  2. toggle back & forth between Smarty3 & Smarty2 with the define define('CIVICRM_SMARTY3_AUTOLOAD_PATH', \path\to\civicrm\ext\smarty3\vendor\autoload.php);`

Most of what this code adds is already put up as separate PRs as it is mergeable now

What the code adds is

  1. a smarty3 extension that really ONLY functions to allow the composer package to be present but condtionally loaded
  2. dev/core##4146 Add Smarty3 Compatibility helper #27585 SmartyCompatibility.phpthis provides a function that allows v3 Smarty functions to run in v2. This could be added to core ASAP allowing us to migrate functions like clear_all_templates() to clearAllTemplates()
  3. Some minor fixes that could be pulled out & merged like not having unneed variables passed by reference (crmScope), , dev/core##4146 Remove unused variables from custom smarty functions #27581 dev/core##4146 Run civix upgrade on afform_html #27579 dev/core##4146 Update Renderer::_tplFetch to work with Smarty3 #27583
  4. the removal of our fetch function override - dev/core##4146 Remove our override of Smartyv2 fetch function #27588 this was our attempt to impose security by raising security whenever fetch received a :string. We have another method that does this parseOneOffStringThroughSmarty & perhaps we could use deprecation notices to find places that should be moved to that to allow us to remove this function. The signture changes
  5. replace use of Smarty.get to get url variables with assigns - these cause notices in smartyv3 (maybe not again i v4?) dev/core##4146 Update debug.tpl to not be noticey with Smarty3 #27582
  6. function smarty_modifier_smarty($string, $class) {` (this is not implemented in v3 the same as v2) is a hack to push out default escaping I think - I did that a while back

After this is installed the most broken things are

  1. Our good old help helpers
    image
  2. pages with backticks dev/core##4146 [NFC] More Smarty 3+ prep, mostly quoting strings and removing backticks #27547 (comment)

Before

After

Technical Details

@larssandergreen with this you can get close to testing out your changes. I'm just trying to figure out if I previously solved 2

Comments

@civibot
Copy link

civibot bot commented Sep 22, 2023

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@larssandergreen
Copy link
Contributor

Seeing a bunch of notices like this:

Notice: Indirect modification of overloaded property CRM_Core_Smarty::$template_dir has no effect in _afform_html_civix_civicrm_config()

Smarty upgrade docs say:
== $cache_dir, $compile_dir, $config_dir, $template_dir access ==
The mentioned properties can't be accessed directly any longer. You must use
corresponding getter/setters like addConfigDir(), setConfigDir(), getConfigDir()

So I guess we need an upgrade to civix for this and something to keep existing extensions working?

@eileenmcnaughton
Copy link
Contributor Author

@larssandergreen this should address that #27579

@larssandergreen
Copy link
Contributor

larssandergreen commented Sep 22, 2023

@eileenmcnaughton We can do that for core extensions, but it's not just that extension, it's any extension that's not updated with civix recently (on my local, that's civirules, civivisualize, angular profiles, log viewer, contribution recur, twilio, iats and mosaico as well). So I guess then we get into a bigger question of how to handle upgrading to Smarty 3 without causing issues with existing extensions.

For example, contribution pages using IATS payment processing are now broken.

@eileenmcnaughton
Copy link
Contributor Author

@larssandergreen yeah - I think the first goal would be to get all the pages working with Smarty3 in core....

Probably worth doing a civix upgrade PR for any that you hit though? I assume they will pick up the mixin?

@larssandergreen
Copy link
Contributor

@eileenmcnaughton Sure, I can do some of those later too.

@eileenmcnaughton
Copy link
Contributor Author

@larssandergreen this search string & CRM_Core_Smarty::singleton seems to work

@JoeMurray
Copy link
Contributor

@Edzelopez could you assign some of these upgrades to our staff, potentially Mary or Lauren or Monish. Lars is it okay if we start with civirules, contribution recur and iats ?

@eileenmcnaughton
Copy link
Contributor Author

@JoeMurray I did put up an IATS pr

@larssandergreen
Copy link
Contributor

@JoeMurray @Edzelopez That would be great. Note that the list of extensions above are just the ones I happen to have had on my local. Any extension with civix version of 23.01 or lower should be upgraded.

@eileenmcnaughton
Copy link
Contributor Author

I just updated this so the define you need (if you git pull which you don't have to) is now CIVICRM_SMARTY3_AUTOLOAD_PATH - will update PR

@eileenmcnaughton eileenmcnaughton force-pushed the smarty-to-go branch 2 times, most recently from ba45071 to 255c344 Compare September 23, 2023 02:05
@JoeMurray
Copy link
Contributor

@Edzelopez let's also go through our extensions starting with line item edit and the grant ones and other financial ones.

@eileenmcnaughton eileenmcnaughton force-pushed the smarty-to-go branch 6 times, most recently from e9ccec8 to cecd68a Compare September 29, 2023 05:50
@eileenmcnaughton
Copy link
Contributor Author

thanks @monishdeb - what do you still use angular profiles for ? I guess it's still part of civivolunteer?

@eileenmcnaughton
Copy link
Contributor Author

Note the stuff in this branch is otherwise merged

@eileenmcnaughton eileenmcnaughton deleted the smarty-to-go branch October 11, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants