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

Add PHP 8.4 support to Smarty #1043

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Visualq
Copy link
Contributor

@Visualq Visualq commented Jul 12, 2024

This pull request updates the code base to support the deprecations introduced in PHP 8.4. It should be backwards compatible.

  1. Updated the function signatures that had implicit null assignment to typed parameters.
  2. Added PHP 8.4 to the 'run tests'-shell script.
  3. Bumped the PHP version in the README.md

@scottchiefbaker
Copy link

Wow... way to stay ahead of things. I support this effort in theory, but is the PHP 8.4 syntax 100% nailed down? Does it make sense to wait until we're 100% sure the syntax isn't gonna change.

I didn't even know PHP 8.4 alpha was out until I saw this issue :)

@Visualq
Copy link
Contributor Author

Visualq commented Jul 13, 2024

Wow... way to stay ahead of things. I support this effort in theory, but is the PHP 8.4 syntax 100% nailed down? Does it make sense to wait until we're 100% sure the syntax isn't gonna change.

I didn't even know PHP 8.4 alpha was out until I saw this issue :)

I ran the test suite against the PHP 8.4 alpha version (no errors/or deprecation warnings). There has been a feature stop and it appears that a lot of 'proposed' deprecations for PHP 8.4 are still being discussed (https://wiki.php.net/rfc/deprecations_php_8_4). Functions that would require attention if all the deprecations are implemented/pass are:

  • md5()
  • sha1()
  • strtok()
  • passing E_USER_ERROR to trigger_error()
  • E_STRICT Constant

The md5() and sha1() could be replaced with hash() however that would bump Smarty's minimum's PHP version to 8.0 or they could be polyfilled but this might be a performance debate. Maybe a simple function_exists could solve this without to much performance degredation

The strtok() is used once within Smarty and can be replaced.

The two deprecations regarding the error constants can easily be solved

This might not be a full PHP 8.4 support update for when it's released. The adjustments are safe down to PHP 7.1.

@eileenmcnaughton
Copy link

I took a look at this & support it being merged. I guess the only contentious part of merging it from my point of view is whether you want to declare php8.4 support - the other changes all seem to be fine as 'preparation' if you are not prepared to go all in

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-packages that referenced this pull request Aug 6, 2024
This patch is from a currently open PR
smarty-php/smarty#1043

I think we can merge this to see our 8.4 start running & be relaxed if we lose it temporarily
if we update Smarty again now before it is merged (which seems unlikely but we can hopefully hustle the merge)
@marclaporte
Copy link
Contributor

is the PHP 8.4 syntax 100% nailed down

The next planned release is RC1 so syntax changes are unlikely at this point.
https://wiki.php.net/todo/php84

@Visualq
Copy link
Contributor Author

Visualq commented Oct 9, 2024

This should cover all the PHP 8.4 deprecations. I ran the tests against the PHP 8.4 RC1 release and besides the small E_STRICT deprecation it works as intended.

I added the PHP 8.4 to CI and, for now, the PHP 8.4 RC1 docker image to the test suite.

One final note:

In smarty-lexer project the following line in Parser.php:
$n = strtok(substr($z, $j), " \t");

can be replaced with:
$n = preg_split("/[ \t]/",substr($z, $j), 2)[0];

to make the lexer php 8.4 compatible aswell, for now this doesn't effect the Smarty library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants