#59846 closed defect (bug) (fixed)
Reinstate the `wpdb::$use_mysqli` property
Reported by: |
|
Owned by: |
|
---|---|---|---|
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.
15 months ago
This ticket was mentioned in PR #5636 on WordPress/wordpress-develop by @johnbillion.
15 months ago
#2
- Keywords has-patch added; needs-patch removed
@johnbillion commented on PR #5636:
15 months ago
#3
#5
@
15 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?
#7
@
15 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.
#9
@
15 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:
↓ 15
@
15 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:
15 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:
15 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:
15 months ago
#13
Sorry, cat on keyboard
@hellofromTonya commented on PR #5636:
15 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
@
15 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
@
15 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
@
15 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
@
15 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.
#21
@
15 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Thank you @jorbin. Backporting now.
@hellofromTonya commented on PR #5636:
15 months ago
#23
Committed via https://core.trac.wordpress.org/changeset/57089 to trunk
and https://core.trac.wordpress.org/changeset/57090 to the 6.4 branch.
I think this would benefit from a unit test to prevent future breakage. I'll work on one shortly.