-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
PHP error on PHP 8 when caching is enabled under a specific edge case #36566
Conversation
I have tested this item ✅ successfully on a58fedc This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36566. |
I have tested this item ✅ successfully on a58fedc This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36566. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36566. |
The fix is incorrect, you should not use implode, but compare array instead. And we already have a fix for this #36068 , please confirm if it works for you. |
Back to pending due to previous comment. Please check. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36566. |
@Fedik Yeah, thanks, I know why it happens (I actually wrote about that in length at the end of the PR). I don't see how the scripts / styles would ever be in a different order since what this code checks is whether a script / style that wasn't there has been appended to the document's head. Joomla can only append to the document head, it cannot inject in a different order. In any case, the point is moot. The other PR is more economical code-wise. I prefer that approach instead of mine. |
Merged the other one |
Summary of Changes
This PR fixes a caching issue which occurs when a module which defines head scripts or styles is being cached after another extension (e.g. template, plugin, or module) has already defined its own, different, head scripts or styles.
This is a type error in Joomla's cache handler which exists in Joomla 3 as well. This PR is for Joomla 4.0.
Tagging @crystalenka (reported the issue to me), @wilsonge and @HLeithner for review.
Testing Instructions
Checkpoint 1
Checkpoint 2
Actual result BEFORE applying this Pull Request
Checkpoint 1
The site works properly and you can see the not cached Foobar module.
Checkpoint 2
Joomla shows an error page (“The requested page can't be found.”) with the following error message:
Expected result AFTER applying this Pull Request
Checkpoint 1
The site works properly and you can see the not cached Foobar module.
Checkpoint 2
The site works properly and you can see the cached Foobar module.
Documentation Changes Required
None whatsoever.
Technical information
The two extensions are dummies which add head styles in the document at different points of the application lifecycle just to trigger the error condition. It should also be possible if you have two DIFFERENT module extensions doing the same, or a module and a template, etc.
The Joomla cache handler (
\Joomla\CMS\Cache\Cache
) has a major type bug in thesetWorkarounds
method. When it compares thescript
andstyle
keys of the document object's head data it incorrectly assumes that they contain string values. In actual reality they always contain EITHER an empty string OR an array.Joomla then calls
\Joomla\String\StringHelper::substr()
and\Joomla\String\StringHelper::strlen()
against the contents of these header keys. If they are non–empty, i.e. the contain an array, this causes a type error.Under PHP 5 and PHP 7 the type mismatch error is a mere PHP warning. You might see it, most likely you won't (because you're likely running your site with error reporting set to None) and caching doesn't work correctly for ‘inexplicable reasons’.
Under PHP 8 type mismatch errors have been upgraded to ErrorExceptions i.e. fatal PHP errors. This is what is caught by Joomla's error handler and why you see an error page instead of the cached module.
My suspicion is that this code was copied from The Olden Days (Joomla 1.x / 2.x and possibly very early 3.x, if memory serves) when Joomla indeed stored a string in the document instead of an array. When the document object was refactored many moons ago this largely undocumented, esoteric piece of code was not updated. The kind of failure it caused was silent under PHP 5 and 7 so any caching issues it caused were attributed to the mythical ‘ghost in the machine’, the cache expired or was reset and everything was good in the world. Come PHP 8 and the root cause, the type mismatch, is now a stop error and suddenly the ghost in the machine grew fangs and bit us in the posterior.
I also suspect without being certain that some Joomla forum posts may have mistakenly blamed an innocent module for this issue, possibly claiming that the plugin is not compatible with PHP 8. Please let me say again that this issue is 100% unrelated to any third or first party extension. It is a bug in Joomla's cache handler. Fun times...