[3.10] [PHP 8.1] Fixes mod_menu/menu.php type null instead of string#36773
[3.10] [PHP 8.1] Fixes mod_menu/menu.php type null instead of string#36773zero-24 merged 2 commits intojoomla:3.10-devfrom
Conversation
Fixes Deprecated: trim(): `Passing null to parameter #1 ($string) of type string is deprecated in administrator/modules/mod_menu/menu.php on line 383`
|
Drone failure here is due to a server issue (database connection), independent of this PR. |
|
I have tested this item ✅ successfully on d53fc78 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36773. |
|
I have tested this item ✅ successfully on d53fc78 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36773. |
|
R2C This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36773. |
|
|
||
| // Exclude if link is invalid | ||
| if (!in_array($item->type, array('separator', 'heading', 'container')) && trim($item->link) === '') | ||
| if (!in_array($item->type, array('separator', 'heading', 'container')) && trim((string) $item->link) === '') |
There was a problem hiding this comment.
| if (!in_array($item->type, array('separator', 'heading', 'container')) && trim((string) $item->link) === '') | |
| if (!in_array($item->type, array('separator', 'heading', 'container')) && empty($item->link)) |
Isnt an empty check better here vs trim and === ''
There was a problem hiding this comment.
Empty will not be true when the string is just a bunch of spaces so we need the trim, or am I wrong?
There was a problem hiding this comment.
With empty also $item->link = null will fail the test here with trim it would have passed this test. But to me $item->link = null should be catched here too right?
There was a problem hiding this comment.
Personal taste: I don't like using empty() as it's basically same as (! (bool) $value) just that if $value doesn't exist, it considers it "empty" too.
https://www.php.net/manual/en/function.empty.php
Additionally, string '0' is considered as empty too.
Additionally, iirc typecastings have slightly changed over the lifecycle of PHP and might change in the future.
So as personal taste, I don't find empty() to be a type-clean function. Thus I'm not willing to take the risk of using empty() as suggested.
My change was a BC safe change because it does exactly what PHP does anyway, without changing anything in the behavior. If casting to (string) is not acceptable, I'm ok to merge a different way, with is_null($item->type) which would then give valid warnings for non-null non-string $item->link.
Updated PR with the slightly cleaner and equivalent fix.
There was a problem hiding this comment.
Side note: After reading again, my reply was way too factual and might read as being hard factual. Please accept my appologizes. I'm non-native English. The comment doesn't convey properly my gratitude. Fixing it here:
Thank you @zero-24 and @richard67 for your feedbacks and aim to have Joomla as clean as possible! 👍
There was a problem hiding this comment.
No problem and thanks for your PRs for PHP8.1
|
Will take this thanks @beat |
Fixes Deprecated: trim():
Passing null to parameter #1 ($string) of type string is deprecated in administrator/modules/mod_menu/menu.php on line 383Pull Request for Issue # none
Summary of Changes
Sometimes item is null, but in PHP 8.1, trim accepts only string type, not null.
Testing Instructions
Code-review could perfectly be enough as it's obvious.
Look at admin area of 3.10, and see error.
Actual result BEFORE applying this Pull Request
Passing null to parameter #1 ($string) of type string is deprecated in administrator/modules/mod_menu/menu.php on line 383
Expected result AFTER applying this Pull Request
That warning disappears.
Documentation Changes Required
None.