Smarty and PHP 7.2

[musicmaster]

I have been looking how I could run my TB 1.08 on PHP 7.2 without those annoying warnings.

My first step was looking for the highest Smarty version that worked with my shop. It was 3.1.27.

Next I looked at the warning that got. I searched the code but there is only one instance of "each()" in it. I found on internet a "myEach()" function that should be able to emulate each() and implemented it.

Attached is the resulting code. Only the file /libs/sysplugins/smartyinternalcompilebase.php has been changed from the default 3.1.27. The code comes under the /vendor/smarty directory of the shop.

As far as I can see it works. Can other people test it? 01546705970018smarty.zip

 

Follow up: it looks like the version-update wasn't a good idea. It seemed to give hangup problems when run under PHP 7.2. However, the modification of smarty_internal_compilebase.php seems to work well. See below.

 

smarty_internal_compilebase.zip

Edited March 9, 2019 by musicmaster

[DorkV89]

I have used your files yesterday and I see a positive change in the Error-log. The site looks like to work fine, so I will leave ity this way.

Thanks!

[Traumflug]

Next I looked at the warning that got. I searched the code but there is only one instance of “each()” in it. I found on internet a “myEach()” function that should be able to emulate each() and implemented it.

Thanks for these efforts.

That said, such workarounds are pretty exactly what I'd like to avoid. The usage of each() you found is a bug and bugs can get solved by changing the buggy code. The PHP folks didn't introduce such warnings to annoy users, but to detect bugs and help eliminating them.

[musicmaster]

@traumflug said in Smarty and PHP 7.2:

That said, such workarounds are pretty exactly what I'd like to avoid. The usage of each() you found is a bug and bugs can get solved by changing the buggy code. The PHP folks didn't introduce such warnings to annoy users, but to detect bugs and help eliminating them.

The fact that a function is deprecated does not mean that it is a bug. The PHP people didn't abolish each() because it is buggy. They just found it a poor alternative for foreach(): slower and harder to read. And given that they wanted to clean up the language a bit it had to go. And if you look around the internet you will see that nearly every major software package used to contain it.

Rewriting the Smarty code with foreach() is a nice ideal but I find their code a bit hard to understand. This was the only alternative to get things working. I find it a bit easy to call it a workaround. I rather see it as risk-averse: given how complicated this piece of their code is it is very easy to get unwanted side effects when you try to restructure it. Just replacing this function minimizes that risk. But of course I will welcome it when at some point in the future some genius manages to restructure this code the "proper" way.

I don't believe upgrading Smarty to a PHP 7.2 compatible version is a viable alternative for TB at the moment. It will break compatibility with almost all the code that was written for Prestashop. And the quality of the error messages means that that is almost impossible to fix.

[Traumflug]

Rule of thumb: never fight the framework.

Updating to a PHP 7.2 compatible Smarty version is actually the only option, short of switching to another framework. Trying to fix older Smarty versions is trying to reanimate a dead horse, and the more efforts go into this, the worse the mess gets. Then thirty bees has not only to deal with Smarty changes, but also to deal with these workarounds.

Smarty should be seen as a given. Actually, even if we wanted, we couldn't put changes to Smarty code into the thirty bees repository, because Smarty isn't part of this repository. It gets fetched as-is during release builds. The only changeable part is the Smarty version number.

Instead of putting efforts into changing something unchangeable, working on the issues coming up with modern Smarty would be much more worthwhile, give faster code, give a future-proof codebase. Which can also result in bug fixes provided to the official Smarty repository, of course.

[musicmaster]

Updating to a PHP 7.2 compatible version of Smarty is the same as switching to another framework. By giving up on backward compatibility in 2015 Smarty abandoned users like Prestashop and Thirty Bees.

Smarty has de facto been abandoned code since 2015. But software doesn't age so we happily have kept using it. With PHP 7.2 we have arrived at a point where one single line in the Smarty code gives a problem. We can now fix that little problem or declare such a fix taboo.

Smarty itself doesn't maintain this code anymore. So we can feel free to do with it whatever we like. There will never be another update from the Smarty maintainers that might interfere with our fix.

It is very nice that the latest Smarty version is better. But so are Twig and other template engines. Switching to each of them will have the same effect at the moment: it will kill Thirty Bees as it will leave it without supporting software.

The argument that the code is fetched doesn't make sense to me. Nothing forbids us to include this code into our own code. Nobody will notice those extra 300K in a software package of 40M.

[the.rampage.rado]

@musicmaster I like your attitude here! :D

[musicmaster]

@the-rampage-rado said in Smarty and PHP 7.2:

@musicmaster I like your attitude here! :D

This is not a small issue. With the Panda theme you cannot access the front office under PHP 7.2 in development mode thanks to this little warning. Very likely it isn't the only theme with such a problem.

Just a few months ago the announcements on the forum were that with the new year Thirty Bees would start supporting PHP 7.2. And now we are told that that will be implemented with a Smarty upgrade that will break backward compatibility - what isn't a real solution.

The plan for Thirty Bees was that it would provide a vision for the future for all the people using Prestashop 1.6 who don't like 1.7. For the short term that meant improvement of the code (more speed, less bugs) and some extra features that provide a concrete incentive for migrating to TB. When enough users had joined would there be the step to TB 1.1 where TB implemented its own vision and gave up on complete backwards compatibility.

The code improvement has gone well and is still going well. But the extra features that should seduce people to migrate are a bit of a problem. Elastic Search appeals to too few people. GDPR may never arrive. And now we are told that PHP 7.2 compatibility won't be what it had been suggested to be either.

[lesley]

Ok, so what we have talked about in the past is to offer a button like prestashop 1.4 did to switch smarty versions. Do you think it would be enough to have a 3 way button that has the two official versions and the one modified version?

[gonssal]

I use https://github.com/cweagans/composer-patches in the two installations I maintain.

[lesley]

@gonssal what are your thoughts about my suggestion? It looks like we would need to use something like what you recommended for it.

[gonssal]

I don't see the need for any button. You just cherry-pick the commits you want and create a patch with them, which is then automatically applied everytime you run Composer.

[lesley]

The need for the button was to offer the later version of smarty that will likely break modules. A push forward so to speak.

[musicmaster]

@lesley said in Smarty and PHP 7.2:

The need for the button was to offer the later version of smarty that will likely break modules. A push forward so to speak.

I don't believe that such a button is a solution. The number of incompatible modules and themes is too large - as far as I can see. And even when a shop might initially work with the new version it might break when the user installs a new module. Most webshop users know little about software and such a crash is for them an absolute taboo. I doubt that many module and theme makers will be prepared to adapt their software - given the still low visibility of Thirty Bees. Many will rather advise their customers to switch to Prestashop.

Prestashop dropped Smarty and I believe that was the right decision. By giving up on backward compatibility Smarty proved itself an unreliable partner for the kind of ecosystem that Prestashop is. Of course sometimes software needs to break backward compatibility. But that should be done with a major new version and support for the old system should be maintained for some time. Smarty didn't do that. What guarantee do we have that they won't do the same again in the future?

When you build a webshop you expect it to be around without major changes for at least three or four years. But on the TB forum people are advised to use PHP 7.1 - that won't be supported after 1 december 2019. Sometimes people are even advised to use PHP 7.0 - that is no longer supported. This is not right.

I see not really an alternative to the kind of solution that I proposed in this post: keeping a compatible Smarty version (that means lower than 3.1.28 as far as I can see) and fixing it so that it runs under PHP 7.2. It is the only way to stay both compatible with the Prestashop 1.6 ecosystem and up-to-date with PHP. My solution was a proof of concept and other people may be able to improve it. But I see no alternative.

[Traumflug]

@musicmaster, would you please stop spreading fear here? thirty bees supports PHP 7.2 already. With development mode turned off, which is (hopefully) true for any production shop, it works just fine.

And I can hardly believe you recommend dropping Smarty here just because there are a few incompatibilities with the newest version. Dropping Smarty certainly makes all modules incompatible. Incompatible in a way which can't get fixed short of rewriting all the modules.

Third, you whine about PHP 7.1 being unsupported next year, while you also try hard to keep a Smarty version which is is unsupported since four years ago now. Totally contradicting.

thirty bees can tackle and will tackle the Smarty problem. There are several strategies available:

• Smarty code is wrapped already, so this wrapper can get enhanced to work around known trouble.
• Themes and modules can get patched on the fly. If there are known showstoppers, create a patch, thirty bees applies it without the merchant even noticing it.
• Theme and module developers are living beings and can certainly enhance their code.
• Smarty is open source, so it accepts patches.
• If all this fails, there's still the option to patch Smarty by composer, as @gonssal mentioned. The newest Smarty version, of course.

With this in mind, all this works much better if people stop working on kludges and start working on the issues actually coming up with a PHP 7.3 compatible version. To cite one of the best thirty bees developers: "I’ve played with new version of smarty, and I’ve never encounter any problem with themes tpl." He said a bit more, still did the right thing: started to search for solutions, not kludges.

Right. I'm back working on GitUpdater. Because the most crucial thing for any kind of code change is a reliable way to transport this code change into shop installations.

[datakick]

I see not really an alternative to the kind of solution that I proposed in this post: keeping a compatible Smarty version (that means lower than 3.1.28 as far as I can see) and fixing it so that it runs under PHP 7.2.

isn't it better to fix the newest smarty version instead?

[datakick]

I have just tested upcoming (not yet released) version of smarty 3.1.35, and there is only one compatibility problem I encountered and that's related to this issue: https://github.com/thirtybees/thirtybees/issues/617.

Fortunately this issue can be fixed. But I'm also sure there will be more problems, as different themes and modules can use different smarty constructs.

Here the help of community is needed. Merchants need to test their themes and modules on new environment and report the problems. TB developers can't fix problems they don't know about. For example, I have no idea why @musicmaster claims smarty 3.1.28 to be the last one usable. What problems did you encounter with higher versions? Are these errors reported and tracked as github issues?

[musicmaster]

@traumflug said in Smarty and PHP 7.2:

@musicmaster, would you please stop spreading fear here? thirty bees supports PHP 7.2 already. With development mode turned off, which is (hopefully) true for any production shop, it works just fine.

Really? Then why recommends the documentation maximal PHP 7.1 - what I also see recommended many times on the forum.

Whenever there is a problem we recommend people to switch on development mode. That is not possible for those people. So I believe this is not a thing you can easily discard.

It is very nice that some smart people are even able to run TB on PHP 7.3. But what I am talking about is what the common webshop owner gets "out of the box". That should work. Period. And it shouldn't be that entirely predictable bugs force him to the forum to get a solution (most people will just jump to WooCommerce or another package). And it should work with PHP 7.2 as otherwise he will within a year find himself with an outdated shop.

So if you have a solution that works now: fine. Please implement it! But if its is still all still in development and will take some time please implement my "kludge" so that TB users can safely use PHP 7.2 now. You can always later implement the system you prefer.

But don't give the users half-finished products with the promise that you will fix it when they find a problem. That is really implementing "kludges".

[lesley]

Just curious, what percent of non thirty bees maintained modules do you think work with php 7.3?

[DaoKakao]

My 5cents: Generally, the problem of compatibility with previous versions isn't decided properly even by software grands.Just remember, that M$ windows or oracle or ibm has tons of very weird/buggy/deprecated/slow/et c. software which is kept version-by-version for the sake of compatibility with tons of important software dependent on their products. Thus they sentenced to keep this plenty of shit for decades. So, let wish a good luck for devteam on their thorny way.

[Traumflug]

Whenever there is a problem we recommend people to switch on development mode. That is not possible for those people [using PHP 7.2].

It is. I run PHP 7.2 myself and get this many times a day. A box with the error report appears (created by xdebug) and if this box gets into the way, one simply reloads the page. These Smarty errors happen during template compilation and once a template is cached, the box no longer appears. Not sure how things appear visually without xdebug, but it certainly works.

[musicmaster]

@lesley said in Smarty and PHP 7.2:

Just curious, what percent of non thirty bees maintained modules do you think work with php 7.3?

No idea. But the changes seem more esoteric as with 7.2. Each() and Count() are very common.

@traumflug said in Smarty and PHP 7.2:

Whenever there is a problem we recommend people to switch on development mode. That is not possible for those people [using PHP 7.2].

It is. I run PHP 7.2 myself and get this many times a day. A box with the error report appears (created by xdebug) and if this box gets into the way, one simply reloads the page. These Smarty errors happen during template compilation and once a template is cached, the box no longer appears. Not sure how things appear visually without xdebug, but it certainly works.

The average webshop owner is not a computer specialist but a commercial person who knows just enough about software. He will install TB on PHP 7.1 - as advised - and at the end of this year be unpleasantly surprised when his hosting provider announces that his account will be upgraded to PHP 7.2.

It is very nice that you know how get around the Smarty error. But how many of the people developing with their Panda theme (that advises to set the switch to "always recompile" while developing) will find that too? Webshop software is a thing that should work out of the box.

[datakick]

I think that thirtybees can't possibly release newest version of smarty for everyone in one big upgrade. That's just too dangerous. While we can fix tb core and modules to work with newest version, we can't guarantee the same for third party modules. So there's need to be some upgrade path.

We should really decouple (some) libraries from core, and let merchants decide which version they would like to use. We could also let merchants to use different lib version in debug mode, so they could test the new versions without the stress.

This is probably the safest way forward. TB should come with some sensible default version that works for majority of users. And once we have enough feedback from merchants that the newer version works fine, we can make it default for everyone (with option to downgrade).

On related note: we should also fix the debug mode. As I understand it, this whole thread is mostly about the warnings and notices that get mixed within HTML (and more importantly JSON) responses. These makes the site unusable. If we overhaul debug mode reporting to something from this millenium, this whole class of problems will cease to exists.

[musicmaster]

@datakick said in Smarty and PHP 7.2:

On related note: we should also fix the debug mode. As I understand it, this whole thread is mostly about the warnings and notices that get mixed within HTML (and more importantly JSON) responses. These makes the site unusable. If we overhaul debug mode reporting to something from this millenium, this whole class of problems will cease to exists.

For me the main point is the PHP 7.1 recommendation. I don't believe that it is a good idea to recommend people at this point of time to start a webshop with that PHP version. Within a year many of them will be automatically upgraded by their hosting provider to PHP 7.2. And as many people apply code changes instead of overrides - so that they cannot easily upgrade TB - that means that quite a few people will have a serious problem.

Starting a webshop based on PHP 7.1 at this point of time is just a bad idea. Unfortunately I don't see much of a sense of urgency on this forum to fix this issue.

[datakick]

@musicmaster said in Smarty and PHP 7.2:

For me the main point is the PHP 7.1 recommendation. I don't believe that it is a good idea to recommend people at this point of time to start a webshop with that PHP version. Within a year many of them will be automatically upgraded by their hosting provider to PHP 7.2. And as many people apply code changes instead of overrides - so that they cannot easily upgrade TB - that means that quite a few people will have a serious problem.

Starting a webshop based on PHP 7.1 at this point of time is just a bad idea. Unfortunately I don't see much of a sense of urgency on this forum to fix this issue.

Do you experience any errors with php 7.2? Or is it just notices / deprecation warnings?