smarty
smarty copied to clipboard
smarty-php
Update and cleanup 4.2.0
I did not touch the sysplugins/smarty_internal_templatelexer.php and smarty_internal_templateparser.php files.
another (smaller) PR for PHP 8.2 (which replaces the https://github.com/smarty-php/smarty/pull/775 by Progi1984) will follow when merged (since building on).
There appear to be a few problems with the unit tests now. Could you look into this? Don't get me wrong: I'm happy with all the effort you put in, but to keep things manageable I'd prefer focused, minimal PRs with a clear goal and a changelog if possible. Much easier to review, too. So maybe you could close this one and provide a couple of separate, focused PRs?
@wisskid
Yes thats the point. Phpunit does not know that I disabled/silenced strftime deprecation "errors" by @ until PHP 9 will arrive in future days and that I did this to avoid using polyfills or other crucial workarounds; strftime will just work fine until then, but we need to avoid the deprecation notices up from PHP 8.1. For phpunit this is an error (w/o value), so the check fails.
This all while the intl extension is a need for the DateTime Interface as the strftime successor per default. And that isn't the case in popular server situations these days.
So I fear I can't do anything against it. Using @ is the simplest solution. Do they (unit checks) stop the merge or are they informational only? Something similar in https://github.com/smarty-php/smarty/pull/775/ I assume, which I would not merge, since my followup is slight different as already noted.
Failing unit tests means no merge, I'm afraid. And AFAIK deprecation notices aren't a show stopper. They exist to inform us ahead of time that some functionality will change in the future. So deprecation notices should not cause unit tests to fail.
Sorry, I have no further insight in github actions and phpunit tests... But it says: This branch has not conflicts... and Only those with write access to this repository can merge pull requests underneath though.
If you take a look youself, it is the last commit which is the @strftime commit that seems to fail. But none of these tests error messages has a value. You said "So deprecation notices should not cause unit tests to fail" but something obviously does.
Maybe that needs something equal for unit test as in PR #775 ? (Please remember, I have a better (extended) one in line for this) I personally never play with these "dependencies", so I can't really say. Maybe you can add it to this pull request?
@ophian if you rebase/merge master you can now easily run the unit tests for each supported PHP version locally if you have docker installed.
For now, 1478 unit tests fail with this error:
Undefined property: Smarty::$smarty
/home/runner/work/smarty/smarty/libs/Smarty.class.php:1311
[...]
@wisskid Yes, thanks. Fixed. Its a strange issue, since the normal ternary does not require a defined $smarty property in the Smarty class for magic __get, while usage with Null Coalescing Operator does.
The here mentioned 3 conflicts with 4.2.1 do only have one related issue with this PR in the libs/plugins/function.html_select_date.php file where I changed the ternary too. Shall I add the new 4.2.1 including my change to this PR or will we do this later?
@ophian there is a lot of value in this PR, but it's too big and all over the place. The use of the null coalescing operator makes the code more readable, but it would be great to have a limited PR that does only that (which could easily been done by cherry-picking 159ed4ee02294d1576e9635ec66b41f63bb1986c).