[3.10] [PHP 8.1] Fixes MVC BaseController Deprecated ucfirst() and Deprecated strtolower() null to string type warnings #36797
Conversation
Fixes `Deprecated: ucfirst(): Passing null to parameter #1 ($string) of type string is deprecated in /home/beat/www/j/libraries/src/MVC/Controller/BaseController.php on line 286` and fixes `Deprecated: strtolower(): Passing null to parameter #1 ($string) of type string is deprecated in /home/beat/www/j/libraries/src/MVC/Controller/BaseController.php on line 684`
|
I have tested this item ✅ successfully on d9c64d8 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/36797. |
zero-24
left a comment
zero-24
left a comment
There was a problem hiding this comment.
hmm i understand what you did here but wouldnt it be more correct to do something like I have done with the suggestions? Else $task != $this->task which does not sound right to me.
We could also question whether we should accept non strings here in the first place.
| */ | ||
| public function execute($task) | ||
| { | ||
| $this->task = $task; |
There was a problem hiding this comment.
| $this->task = $task; | |
| $this->task = (string) $task; |
There was a problem hiding this comment.
I have no idea how the other parts of the code would react with $this->task becoming a string '' instead of null. This is why I have suggested this fix with zero bug risks (keeping in mind this is for 3.10 and in 18 months, it's EOL).
getTask() is called from 26 different places in Joomla, and probably from many third-party developers as well. Having it suddenly return a string '' instead of null in a maintenance release seems as a not so great idea.
I'm not inclined to accept this change-suggestion, for the sake of avoiding potential breaks..
| $this->task = $task; | ||
|
|
||
| $task = strtolower($task); | ||
| $task = strtolower((string) $task); |
There was a problem hiding this comment.
| $task = strtolower((string) $task); | |
| $task = strtolower($this->task); |
There was a problem hiding this comment.
Same comment as above:
I have no idea how the other parts of the code would react with $this->task becoming a string '' instead of null. This is why I have suggested this fix with zero bug risks (keeping in mind this is for 3.10 and in 18 months, it's EOL).
getTask() is called from 26 different places in Joomla, and probably from many third-party developers as well. Having it suddenly return a string '' instead of null in a maintenance release seems as a not so great idea.
I'm not inclined to accept this change-suggestion, for the sake of avoiding potential breaks..
|
Merging. Thanks. |
4 participants
Pull Request for Issue # none
Summary of Changes
Fixes MVC BaseController for warnings:
Deprecated: ucfirst(): Passing null to parameter #1 ($string) of type string is deprecated in /home/beat/www/j/libraries/src/MVC/Controller/BaseController.php on line 286and
Deprecated: strtolower(): Passing null to parameter #1 ($string) of type string is deprecated in /home/beat/www/j/libraries/src/MVC/Controller/BaseController.php on line 684Testing Instructions
Code review is probably enough, as trivial.
Check frontend with PHP 8.1 with all errors on and Joomla Debug ON.
Actual result BEFORE applying this Pull Request
Deprecated: ucfirst(): Passing null to parameter #1 ($string) of type string is deprecated in /home/beat/www/j/libraries/src/MVC/Controller/BaseController.php on line 286Deprecated: strtolower(): Passing null to parameter #1 ($string) of type string is deprecated in /home/beat/www/j/libraries/src/MVC/Controller/BaseController.php on line 684Expected result AFTER applying this Pull Request
Those warnings disappeared.
Documentation Changes Required
None.