-
Notifications
You must be signed in to change notification settings - Fork 717
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
address PHP 8.1 'explode', 'number_format', and 'replace' deprecations #755
Conversation
This seems great at first glance, and all the tests seem to be passing. However, it feels a bit weird to start providing wrappers for native PHP-functions, only to prevent deprecation errors that result from these very PHP-functions. I can see why one might want this, but where is this going to end? Are we going to be writing wrapper plugins to circumvent each and every change in PHP from now on? |
For me I simply wasn't able to think of another way to address the reported items in #750. I felt this approach was similar in spirit to changes like #743 in providing a default which preserves existing behavior, but I do see the potential headache that could stem from creating a layer in front of PHP built-ins. If there is a different approach that would be preferred or if the decision is just not to address these deprecation issues at all then let me know and I'll amend or close the PR as needed. |
@wisskid Yes, I think that's what a template engine should do. Whatever version of PHP it runs on, it works the same. However strict PHP decides to get, Smarty decides its own strictness. I think that's only possible with wrappers like this. |
@mfettig @rudiedirkx you are both right. I've given it some thought, and I think we should go forward in the direction you propose. One way out of the never ending chore of keeping up with changes in PHP could be to drop support for passthrough of modifiers to native PHP functions in Smarty 5. We could provide a set of standard modifier plugins to cover the most common cases. The ones provided by @mfettig would be a good start, I think. |
tests/UnitTests/TemplateSource/TagTests/PluginModifier/PluginModifierExplodeTest.php
Outdated
Show resolved
Hide resolved
tests/UnitTests/TemplateSource/TagTests/PluginModifier/PluginModifierNumberFormatTest.php
Outdated
Show resolved
Hide resolved
4abef5f
to
5234d33
Compare
@wisskid I've removed the setDebugging() calls and rebased this branch onto the current master branch to resolve a conflict that had cropped up |
tests/UnitTests/TemplateSource/TagTests/PluginModifier/PluginModifierNumberFormatTest.php
Outdated
Show resolved
Hide resolved
5234d33
to
4107b7e
Compare
This PR is meant to address several of the PHP 8.1 deprecation items noted in Issue #750
It does not address the deprecation noted in 'escape' because I was not able to recreate the issue against the current master branch
It also does not address any strftime() related deprecation issues since it is not clear to me how that issue should be addressed as a whole. Discussions were present in #678 and touched on using an intl polyfill, but changes were deferred at that time. #672 is open as a strftime() specific issue so I suppose it would still be handled there unless a decision has already been made, in which case I would be happy to update this PR once I know how it should be handled.