RTC PR-3.10-dev PHP 8

Pending

User tests: Successful: Unsuccessful:

avatar beat
beat
22 Jan 2022

Pull Request for Issue # none.

Summary of Changes

Fixes Deprecated: mysqli_real_escape_string(): Passing null to parameter #2 ($string) of type string is deprecated in libraries/joomla/database/driver/mysqli.php on line 254

Testing Instructions

Source-code review should be enough here.

Just test that site continues to work.
I saw this error while saving a user in admin aera with CB.

Actual result BEFORE applying this Pull Request

Deprecated: mysqli_real_escape_string(): Passing null to parameter #2 ($string) of type string is deprecated in libraries/joomla/database/driver/mysqli.php on line 254

Expected result AFTER applying this Pull Request

That warning is not displayed

Documentation Changes Required

None.

avatar beat beat - open - 22 Jan 2022
avatar beat beat - change - 22 Jan 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 Jan 2022
Category Libraries
avatar beat beat - change - 22 Jan 2022
Title
[3.10] [PHP 8.1] Mysqli database driver escapt function fix
[3.10] [PHP 8.1] Mysqli database driver escape function fix
avatar beat beat - edited - 22 Jan 2022
avatar richard67
richard67 - comment - 22 Jan 2022

I have tested this item โœ… successfully on a807845

Code review.


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

avatar richard67 richard67 - test_item - 22 Jan 2022 - Tested successfully
avatar richard67
richard67 - comment - 22 Jan 2022

@beat Does it need to be fixed in the other drivers, too?

And for 4.x it needs to make a PR with the same fix here: https://github.com/joomla-framework/database . Will you do that, or shall I?

avatar richard67
richard67 - comment - 22 Jan 2022

And should there maybe be a type cast to string for the call to addcslashes in line 258?

Ah no, I see it's not necessary since already handled before.

avatar beat
beat - comment - 22 Jan 2022

All my PHP 8.1 fixes should be checked if they need to also be done in other Joomla branches as well. Same also as you well asked and checked, if they should be done in other files as well.

At this time, I'm just testing Joomla and CB on PHP 8.1 in all functionality, and fixing in parallel in CB, CB add-ons and Joomla 3.10 and 4.0 things I see (just looking around the fix itself if nearby functions need the same fix). And I guess that I will have a few more days of work to do so at least, way more than I never had to do in a previous PHP new version since PHP 4!

PHP made this strange decision to deprecate null as string in ancient string functions, breaking many code that worked in 8.0. Actually, Iย think it's not a good decision, ancient string functions should be left alone, and a new string class, like in many OO languages be made native and implicit (e.g. "text".strlen()). Is there a PHP RFC or discussion for that ? is it too late to comment against this for removal in PHP 8.2 ? it will break unneededly a massive amount of PHP code, just because strlen() and other string functions accept only string and not ?string. Or is null value going to be removed alltogether in PHP 9ย or10 ?

So, if you have time to check and fix if needed other branches and files, it would be awesome and then just comment in each fix that you checked it and result (as you did above for other functions), so that we all know. If not, I might do it at some time when I catched up other tasks as this takes way longer than planed. Thank you for your reviews and help in all cases ๐Ÿ‘

avatar richard67
richard67 - comment - 22 Jan 2022

I don't know if I have the time to check everything on other branches, but I can handle the database driver stuff.

avatar richard67
richard67 - comment - 22 Jan 2022

@beat Please check beat#5 for the same for for the other db drivers.

avatar richard67
richard67 - comment - 22 Jan 2022

For the framework see joomla-framework/database#260 .

avatar beat
beat - comment - 22 Jan 2022

@beat Please check beat#5 for the same for for the other db drivers.

Looks good on code review ๐Ÿ‘

avatar beat beat - change - 22 Jan 2022
Labels Added: PR-3.10-dev PHP 8
avatar joomla-cms-bot joomla-cms-bot - change - 22 Jan 2022
Category Libraries Libraries Postgresql
avatar richard67
richard67 - comment - 22 Jan 2022

I have tested this item โœ… successfully on 2bfda40

Code review.


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

avatar richard67 richard67 - test_item - 22 Jan 2022 - Tested successfully
avatar alikon
alikon - comment - 23 Jan 2022

I have tested this item โœ… successfully on 2bfda40


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

avatar alikon alikon - test_item - 23 Jan 2022 - Tested successfully
avatar alikon alikon - change - 23 Jan 2022
Status Pending Ready to Commit
avatar alikon
alikon - comment - 23 Jan 2022

RTC


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

avatar zero-24
zero-24 - comment - 23 Jan 2022

Hmm I dont like that we try to cast stuff to strings where the original input should have been an string in the first place. So I would like to understand where the issue is comming from, do we have a stack trace?

But I guess we have to do it here to not break the int and float case right?

avatar richard67
richard67 - comment - 23 Jan 2022

Hmm I dont like that we try to cast stuff to strings where the original input should have been an string in the first place. So I would like to understand where the issue is comming from, do we have a stack trace?

But I guess we have to do it here to not break the int and float case right?

@zero-24 I've worried especially about the int case, see the additional comment in the description of the PR here: joomla-framework/database#260 .

avatar zero-24
zero-24 - comment - 23 Jan 2022

Will merge this here as workaround for 3.10. But yes I agree with you that we should find a better solution upstream within J4

avatar zero-24 zero-24 - change - 23 Jan 2022
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-01-23 17:21:39
Closed_By zero-24
Labels Added: RTC
avatar zero-24 zero-24 - close - 23 Jan 2022
avatar zero-24 zero-24 - merge - 23 Jan 2022
avatar beat
beat - comment - 23 Jan 2022

@zero-24 here the stack trace (saving a user in admin with CB):

image

CB is calling the save() function without any parameters, so it's the defaults public function save($updateOnly = false).

Didn't see a less B/C-risky way to fix it.

avatar beat
beat - comment - 23 Jan 2022

Oh, just saw it got merged. All ok then!

Add a Comment

Login with GitHub to post a comment