Make WordPress Core

Opened 4 weeks ago

Closed 4 weeks ago

Last modified 4 weeks 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.


4 weeks ago

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


4 weeks ago
#2

  • Keywords has-patch added; needs-patch removed

@johnbillion commented on PR #5636:


4 weeks ago
#3

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

#4 @johnbillion
4 weeks ago

  • Keywords needs-unit-tests added

#5 @joemcgill
4 weeks 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
4 weeks ago

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

#7 @johnbillion
4 weeks 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
4 weeks ago

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

#9 @jrf
4 weeks 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
4 weeks 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:


4 weeks 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:


4 weeks 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:


4 weeks ago
#13

Sorry, cat on keyboard

@hellofromTonya commented on PR #5636:


4 weeks 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
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks 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
4 weeks ago

  • Keywords dev-reviewed added

#21 @hellofromTonya
4 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Thank you @jorbin. Backporting now.

#22 @hellofromTonya
4 weeks 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.


4 weeks ago

Note: See TracTickets for help on using tickets.