Make WordPress Core

Opened 5 months ago

Closed 5 months ago

Last modified 5 months ago

#59846 closed defect (bug) (fixed)

Reinstate the `wpdb::$use_mysqli` property

Reported by: johnbillion's profile johnbillion Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.4.1 Priority: normal
Severity: normal Version: 6.4
Component: Database Keywords: has-patch has-unit-tests commit dev-reviewed
Focuses: Cc:

Description

In #59118/[56475] the wpdb::$use_mysqli property was removed because it was a private property that was no longer accessed from within wpdb. This should mean its removal was safe, however the wpdb class also includes a __call() magic method which exposes private and protected properties. This means accessing $wpdb->use_mysqli is actually possible and is in use by plugins.

See comments on #59118 for discussion.

To retain backwards compatibility, the property should be reinstated with a value of true.

Change History (24)

This ticket was mentioned in Slack in #core by jorbin. View the logs.


5 months ago

This ticket was mentioned in PR #5636 on WordPress/wordpress-develop by @johnbillion.


5 months ago
#2

  • Keywords has-patch added; needs-patch removed

@johnbillion commented on PR #5636:


5 months ago
#3

I think this would benefit from a unit test to prevent future breakage. I'll work on one shortly.

#4 @johnbillion
5 months ago

  • Keywords needs-unit-tests added

#5 @joemcgill
5 months ago

Nice detective work. I saw reports of this last night and wondered how the private method was being accessed. It makes me question the value of marking those properties as private in the first place. Regardless, I wonder if we should throw a deprecation warning if someone tries to access the mysqli property directly?

#6 @jrf
5 months ago

@johnbillion __call() exposes private/protected methods, not properties.

#7 @johnbillion
5 months ago

@jrf Ah yes somehow I went from __get() to __call(). Thanks.

@joemcgill I don't know if a deprecation warning provides much value. I would be happy just to reinstate the property with a value of true.

#8 @johnbillion
5 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#9 @jrf
5 months ago

@johnbillion Looks like there are a set of magic __get()/__set()/__isset()/__unset() methods though, which do expose private/protected properties, so the issue is valid.

#10 follow-up: @joemcgill
5 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

I would also be happy to just reinstate the property, but was thinking it might provide more info to developers that they should remove these calls from their code, since it's no longer in use, and would better signal to future core devs that the property is present for back-compat, but no longer in use. That said, I don't feel strongly about it.

@hellofromTonya commented on PR #5636:


5 months ago
#11

I wonder if we should add a conditional in the get() magic method that would return a deprecation warning if this property is accessed?

@hellofromTonya commented on PR #5636:


5 months ago
#12

I wonder if we should add a conditional in the get() magic method that would return a deprecation warning if this property is accessed?

@hellofromTonya commented on PR #5636:


5 months ago
#13

Sorry, cat on keyboard

@hellofromTonya commented on PR #5636:


5 months ago
#14

I wonder if we should add a conditional in the get() magic method that would return a deprecation warning if this property is accessed?

Could, but then should also look at the other magic methods too. Thinking for this quick fix, restoring the property makes the most the sense.

#15 in reply to: ↑ 10 @hellofromTonya
5 months ago

Replying to joemcgill:

I would also be happy to just reinstate the property, but was thinking it might provide more info to developers that they should remove these calls from their code, since it's no longer in use, and would better signal to future core devs that the property is present for back-compat, but no longer in use. That said, I don't feel strongly about it.

Restoring the property resolves the BC break. While triggering wp_trigger_error() within the __get() / __set() / __unset() / __isset() is possible, IMO it's not necessary and would add more code to be maintained.

#16 @hellofromTonya
5 months ago

  • Keywords has-unit-tests commit added; needs-unit-tests removed

Patch: https://github.com/WordPress/wordpress-develop/pull/5636

Restoring the property resolves the BC break. The test helps for regression checks.

LGTM for commit.

#17 @rajinsharwar
5 months ago

Manual Testing info, and error logging if the property is accessible by the plugins.

global $wpdb;

// Check if the $wpdb object is available, and error log accordingly
if (isset($wpdb) && is_object($wpdb)) {
    if (property_exists($wpdb, 'use_mysqli')) {
        $useMysqli = $wpdb->use_mysqli;
        error_log('MySQLi driver is ' . ($useMysqli ? 'enabled' : 'disabled'));
    } else {
        error_log('$wpdb->use_mysqli property does not exist');
    }
} else {
    error_log('$wpdb object is not available');
}

#18 @hellofromTonya
5 months ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Thank you everyone. I'll go ahead to commit https://github.com/WordPress/wordpress-develop/pull/5636.

#19 @hellofromTonya
5 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 57089:

Database: Reinstate wpdb::$use_mysqli property.

Partial revert of [56475] to reinstate the private wpdb::$use_mysqli property and set to its default to true. This private property was / is accessible through the magic methods. Though Core's usage of this property was removed by [56475], plugins are using the property. Reinstating it resolves the BC break.

Follow up to [56475].

Props jason_the_adams, joemcgill, johnbillion, johnjamesjacoby, jrf, rajinsharwar, renehermi.
Fixes #59846.

#20 @jorbin
5 months ago

  • Keywords dev-reviewed added

#21 @hellofromTonya
5 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thank you @jorbin. Backporting now.

#22 @hellofromTonya
5 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 57090:

Database: Reinstate wpdb::$use_mysqli property.

Partial revert of [56475] to reinstate the private wpdb::$use_mysqli property and set to its default to true. This private property was / is accessible through the magic methods. Though Core's usage of this property was removed by [56475], plugins are using the property. Reinstating it resolves the BC break.

Follow up to [56475].

Reviewed by jorbin.
Merges [57089] to the 6.4 branch.

Props jason_the_adams, joemcgill, johnbillion, johnjamesjacoby, jrf, rajinsharwar, renehermi.
Fixes #59846.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


5 months ago

Note: See TracTickets for help on using tickets.