Version 4.5.1 breaking change: Variables by reference

[ssigwart]

This is similar to #961, but I couldn't tell if that's specific to version 5 or not.

Smarty 4.5.1 introduces a breaking change regarding parameter references. For example, the following code in Smarty 4.4.1 works:

{assign var="match" value=null}
{if preg_match('/([a-z]{4})/', 'a test', $match)}
	{$match.1}
{/if}

This will output "test".

However in 4.5.1, it outputs this:

Warning: preg_match(): Argument #3 ($matches) must be passed by reference, value given in ...

This seems related to the change in libs/sysplugins/smarty_internal_templatecompilerbase.php from c7a2713.

[wisskid]

@ssigwart is it correct that the error only occurs if you register preg_match as a plugin, i.e. something like $smarty->registerPlugin('modifier', 'preg_match', 'preg_match');?

[ssigwart]

@wisskid, yes, we call registerPlugin for preg_match. If I remove that, the line code properly, but then something like {'/and/'|preg_match:'a and b'} give an Using unregistered function "preg_match" in a template is deprecated and will be removed in a future release error.

[wisskid]

@ssigwart OK. I'm working on a solution for the "argument must be passed by reference" error. For now, you can continue without registering the preg_match as a plugin. The deprecation notice you are seeing is harmless and should not be triggered if you have your error settings configured properly in production.

[ssigwart]

Thanks, @wisskid. At my company, we log deprecation warnings and getting flooded with them is not an option as it will make it very difficult to upgrade other packages or PHP itself. At the moment, we'd decided that we won't upgrade above version 4.4.1 and will likely fork from there if we need to make updates to support future PHP versions or security fixes. See #967 (reply in thread) for more context. I did want to report this issue though so it doesn't impact others because I was fortunate to stumble upon this before moving from 4.4.1 to 4.5.1.

[wisskid]

@ssigwart can you please confirm that dev-support/4 fixes your problem? You can run composer require smarty/smarty:dev-support/4 in your dev environment to test.

[ssigwart]

@wisskid, yep, that works. Thanks! When you merge that, would you mind if I submit a PR to add disableVersion5UpgradeDeprecations() that disables the errors?

[wisskid]

@ssigwart why? Deprecations have been invented for a reason. Instead of calling a disableVersion5UpgradeDeprecations, one can also just lower error reporting level. In fact, that's easier as it is probably already the default for production.

[ssigwart]

I don't want to lower the error reporting level. One place I use them extensively is with PHP version updates. My company tries to clean up all deprecation notices we see. We have a very large code base, when we go from say PHP 8.1 to 8.2, we run it locally for a little while and clean up deprecations as we come across them. Then when we go to production with it, we check the logs and clean up the ones that we didn't find. Typically, there's a big influx and we clean them up, but over the next months we get handfuls of them for things that are rarely used features or really edge cases. For Smarty, at least at the moment, I can't see us upgrading to version 5 and trying to clean up all these deprecation warnings will take a significant amount of time. So my options are to stick with version 4.4.1 and hope that there's no security patches required or deal with deprecation warnings that I know won't have an impact unless we move to version 5 at some point. When we do that, then I'd deal with all the places that truly need updating versus now where we'd be updating a bunch of code just because there's a deprecation warning about a future version we don't have plans for.

[wisskid]

Well, that sounds great, as this is indeed exactly what the notices are for. What I fail to understand however is why you are happy to handle the PHP deprecation notices, but not with the (few) ones Smarty generates. How are they different?

[ssigwart]

why you are happy to handle the PHP deprecation notices

Ha. I wouldn't say happy about it, but it's something we need to do in order to keep PHP up to date.

but not with the (few) ones Smarty generates. How are they different?

If we were moving to Smarty 5, we would handle the deprecations. In general, handling deprecations isn't what we want to be doing. We want to be adding new features to our website. What makes this different is that Smarty 4.5.0 seems like it should be a minor version update from 4.4.1, but it adds deprecation warnings. So the only reason to spend time now trying to find and clean up the deprecations is to quiet the library. I totally get cleaning up the code if/when we move to version 5, but we can plan that.

Given the size of our codebase, I don't think this will be just a "few" warnings and any bulk changes seem risky.

Another difference is that PHP is a really core piece of technology we use. Smarty is pretty core too, but less so and it's hard to convince my company that we're going to need to spend development time upgrading when there's no visible impact to them versus new features. At least with a PHP upgrade, we can point to the typical performance enhancements.

Not sure if it means anything, but we were in the unfortunate situation of launching on Smarty 2 in 2009, which happens to be just before Smarty 3 was released. Smarty 3 was a big change from Smarty 2. We used a slightly self patched version of Smarty 2 until last fall, when we finally finished a many years' long project to update and test all pages in our application to work with Smarty 3 and 4 (fun note... we were midway through working towards Smarty 3 when 4 came out, but at least that was a smaller delta).

[ssigwart]

@wisskid, I see that you released version v4.5.5 with PHP 8.4 support, which is great, but I'm still avoiding upgrading version 4 passed v4.4.1 due to the deprecations notices that were added. I would really prefer not to have to create a fork if I don't need to. Is there any chance you'd reconsider merging a PR if I submitted one to add disableVersion5UpgradeDeprecations.

[wisskid]

No, I think Smarty should not have to provide a setting to disable deprecation notices, just like PHP doesn't have to. If you choose to ignore the deprecation messages because you don't plan to upgrade to the next version, that's totally fine. But if handling deprecations isn't what you want to be doing, that's fine too. Just let them be. You should never log them in production and if the messages bother you during development, I'd suggest that you change your error handling / reporting / logging instead of creating a fork of Smarty.

PS. If it helps: Smarty 5.4 is considerably faster than v4.3, especially if you upgrade your deprecated code.

[ssigwart]

Thanks for your opinion, @wisskid. It sounds like forking will be our patch of least resistance. My company has a bit of a catch 22 situation here. We can't upgrade to version 5 because assignByRef was removed and we have over 5,000 usages of that. I can't justifying spending lots of time and introducing risk to clean up these deprecations warnings which are only there to help people who will upgrade to version 5. I also am affected by #1028 in version 4.

You should never log them in production

If you are referring to showing them to public users, I totally agree for security reasons. That applies to any error level. However, if you are referring to logging them to a file so I can review them, I disagree. With a massive codebase, logging deprecation warning to a file in production is the only viable way to find the extreme edge cases that you won't likely find in development.

[wisskid]

If you are referring to showing them to public users, I totally agree for security reasons. That applies to any error level. However, if you are referring to logging them to a file so I can review them, I disagree. With a massive codebase, logging deprecation warning to a file in production is the only viable way to find the extreme edge cases that you won't likely find in development.

You should (indeed) never show your errors in production. But logging deprecation notices in production is not best practice either: https://stackoverflow.com/questions/50602776/php-error-reporting-production-vs-development
However, the choice is yours, of course.