Skip to content
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

Closed
wants to merge 2 commits into from
Closed

PHP error on PHP 8 when caching is enabled under a specific edge case #36566

wants to merge 2 commits into from

Conversation

nikosdion
Copy link
Contributor

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

  • Install a new Joomla 4 site. Make sure you are using PHP 8.0
  • Login to the backend of your site
  • Go to System, Install, Extensions
  • Install the Foobar test plugin
  • Install the Foobar test module
  • Go to System, Manage, Plugins and enable the System - Foobar plugin
  • Go to Content, Site Modules, click on New
  • Choose the “Foobar Test Module”
  • Use the following information
    • Title: Test
    • Position: Main-top [main-top]
  • Click on Save & Close
  • Go to your site's public frontend

Checkpoint 1

  • Back in the backend of your site, go to Home Dashboard and click on Global Configuration
  • Click the System tab and set System Cache to ON - Progressive caching
  • Click on Save & Close

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:

If difficulties persist, please contact the website administrator and report the error below.

0 mb_strlen(): Argument #1 ($string) must be of type string, array given

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 the setWorkarounds method. When it compares the script and style 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...

@toivo
Copy link
Contributor

toivo commented Jan 5, 2022

I have tested this item ✅ successfully on a58fedc

Tested successfully in 4.0.6-dev of January 4 using PHP 8.0.13 and 8.1.0 in Wampserver 3.2.6


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36566.

@crystalenka
Copy link
Member

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.

@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36566.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 5, 2022
@Fedik
Copy link
Member

Fedik commented Jan 5, 2022

The fix is incorrect, you should not use implode, but compare array instead.
(Use of Implode will have different result for the same scripts in defferent order).
it nothing with PHP. It because Joomla 4 use array to store each script, while Joomla 3 a string.

And we already have a fix for this #36068 , please confirm if it works for you.

@richard67
Copy link
Member

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.

@richard67 richard67 removed the RTC This Pull Request is Ready To Commit label Jan 5, 2022
@nikosdion
Copy link
Contributor Author

@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.

@nikosdion nikosdion closed this Jan 5, 2022
@wilsonge
Copy link
Contributor

wilsonge commented Jan 5, 2022

Merged the other one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants