[#39112] - Fixing PHP 8 error when $list is not array
- Fixed in Code Base
- 2 Nov 2022
- Medium
- Build: 4.2-dev
- # 39112
- Diff
- crystalenka:fix/php8-error-list
User tests: Successful: 0 Unsuccessful: 0
Summary of Changes
Teensy tiny PR. In PHP 8, count() is only valid if the object passed is countable, otherwise it results in an error: https://www.php.net/manual/en/function.count.php
In circumstances that I have not yet been able to divine, sometimes the $list variable in mod_menu.php is boolean. I've seen this in the wild on one of my client sites; it breaks a few times a day based on the emails I'm getting...
A PHP Exception occurred on your site. Here you can find the stack trace:
Exception Type: TypeError
File: /public_html/modules/mod_menu/mod_menu.php
Line: 26
Message: count(): Argument #1 ($value) must be of type Countable|array, bool given
Testing Instructions
I am not sure how to reproduce the actual problem that's making $list pass as boolean; it does not happen consistently. So, I'm requesting this PR be tested by code review since it's so small and does prevent a PHP 8 error.
Actual result BEFORE applying this Pull Request
If $list is boolean, the site breaks with a PHP exception.
Expected result AFTER applying this Pull Request
The site does not completely break, but the menu may not render.
Link to documentations
Please select:
-
Documentation link for docs.joomla.org:
-
No documentation changes for docs.joomla.org needed
-
Pull Request link for manual.joomla.org:
-
No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Modules Front End |
| Labels |
Added:
?
|
||
You can write if (!empty($list)) or if($list) as we do in other modules.
Example here: https://github.com/joomla/joomla-cms/blob/4.2-dev/modules/mod_articles_news/tmpl/default.php#L15
Done
I have tested this item
If someone wants to reproduce the error: Change line 16 to $list = '';
Then $list is empty and the error is visible bafore patch and hase gone after patch.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39112.
I have tested this item
✅ successfully on 7949d9cIf someone wants to reproduce the error: Change line 16 to $list = ''; Then $list is empty and the error is visible bafore patch and hase gone after patch.
I had to update to get the build to run successfully, would you test again?
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39112.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39112.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/39112.
| Labels |
Added:
?
|
||
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-11-02 08:21:57 |
| Closed_By | ⇒ | laoneo |
Thanks!
If I was guessing from the code by the way likely cause would be a race condition on the cache - is the website cachetime out about the same frequency as you getting the exceptions?
@wojtekxtx if you don't know then no one has to waste their time explaining thingss to you UNLESS the question directly involves you.
any plan to add this in v3?
and all the other modules that might need it, like:
https://github.com/joomla/joomla-cms/blob/4.2-dev/modules/mod_articles_news/tmpl/default.php
vs.
https://github.com/joomla/joomla-cms/blob/3.10-dev/modules/mod_articles_news/tmpl/default.php
J3 is EOL and does not get any bugfixes anymore. We only support security fixes until August next year. This means that J3 wont be fully supported on PHP8.1
Thanks for the clarification. I assumed PHP support was under security support.
I assume this also means you will not accept PR's against J3 that fixes PHP support? (not a big issue for me, I will just fork J3 and maintain PHP 8.x support there).
Indeed. If you want to stay on J3 and use PHP8.1, you would have to fork the project. However I would strongly suggest upgrading to J4. There is just so much stuff that has been improved in J4...
Instead of forking the project i would recommend to do an override on the articles news module when its used and you need PHP 8.1 support. ;)
You can write if (!empty($list)) or if($list) as we do in other modules.
Example here: https://github.com/joomla/joomla-cms/blob/4.2-dev/modules/mod_articles_news/tmpl/default.php#L15