#56198 closed enhancement (fixed)
Check cache key type
Reported by: | tillkruess | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.1 | Priority: | normal |
Severity: | normal | Version: | 6.1 |
Component: | Cache API | Keywords: | has-patch has-unit-tests needs-dev-note |
Focuses: | performance | Cc: |
Description
It's unfortunately relatively common for plugins to call the wp_cache_*()
methods with an empty-string or false
as cache key, usually as a result of not checking the return value of another function that's used as the key.
Currently this is failing silently and thus leads on odd behavior at best and often breakage.
$sessionId = session_id(); // equals "" when session wasn't started
wp_cache_get( $sessionId );
Doing a quick type check on the given cache keys and calling _doing_it_wrong()
would help plugins developers to notice these issues quicker.
Change History (30)
This ticket was mentioned in PR #2967 on WordPress/wordpress-develop by tillkruss.
2 years ago
#1
- Keywords has-patch added
#3
@
2 years ago
+1 as I outlined in the ticket I opened a few weeks ago, those are the bain of my existence, since even plugins with millions of users have this issue
#4
@
2 years ago
- Milestone changed from Awaiting Review to 6.1
Thanks for the patch! I have a few notes:
- It looks like the conditional is reversed in individual methods, causing unit tests to fail:
if ( $this->_valid_key( $key ) ) { return false; }
This should be:if ( ! $this->_valid_key( $key ) ) { return false; }
- The
'empty string'
part should be translatable too, but it's not. I think the notice could be split into two for clarity:- For empty strings:
Cache key must not be empty.
- For other types:
Cache key must be integer or non-empty string, %s given.
- For empty strings:
- A translator comment is needed for the string, something like:
/* translators: %s: The type of the given cache key. */
- We don't use
debug_backtrace()
like that anywhere else in core, I think for performance reasons. We might need to pass the caller method name to::_valid_key()
instead, or move the message to the calling methods.
#5
follow-up:
↓ 6
@
2 years ago
Cache key must not be empty.
I think this is confusing, because 0 and '0' are valid cache keys but technically (in PHP terms) 'empty'.
I think the single error string that is in the PR now is better.
===
Regarding translation:
Afaik l10n.php might not be loaded yet when this function is called at all. Since the object cache is loaded before l10n.php.
I think a function_exists() check for all translations functions in class-wp-object-cache.php is needed.
===
if ( is_string( $key ) && trim( $key ) !== '' ) {
I think trimming is a good idea to disallow space only keys.
However I would do the same for groups then too, to keep things consistent.
if ( empty( $group ) ) {
=> if ( empty( trim( $group ) ) ) {
#6
in reply to:
↑ 5
@
2 years ago
Replying to malthert:
Cache key must not be empty.
I think this is confusing, because 0 and '0' are valid cache keys but technically (in PHP terms) 'empty'.
Good point, I would revise the suggestion then:
- For empty strings:
Cache key must not be an empty string.
- For other types:
Cache key must be integer or non-empty string, %s given.
I think the single error string that is in the PR now is better.
It may be if we find a way to improve the wording, but we should not insert English phrases like 'empty string'
into a translatable string, it results in a mix of languages that is neither English nor properly translated.
Regarding translation:
Afaik l10n.php might not be loaded yet when this function is called at all. Since the object cache is loaded before l10n.php.
I think a function_exists() check for all translations functions in class-wp-object-cache.php is needed.
Good point too! Translation functions might indeed not be included if WordPress is loaded in SHORTINIT
mode. We could do a function_exists( '__' )
check similar to the one found in the _doing_it_wrong()
function itself:
if ( function_exists( '__' ) ) { $message = sprintf( __( 'Cache key must be integer or non-empty string, %s given.' ), $type ); } else { $message = sprintf( 'Cache key must be integer or non-empty string, %s given.', $type ); }
Another option might be to skip translation altogether.
#8
@
2 years ago
@SergeyBiryukov @malthert: Thanks for the suggestions, I've implemented them all and tried to make the code as readable as possible given the nesting possibilities.
I've kept the debug_backtrace()
call for now, because passing in __METHOD__
like @malthert says seems dumb.
@spacedmonkey asked about floats on GitHub, should we simply disallow them, or cast them to strings (seems risky to me)?
#9
@
2 years ago
5 tests were failing because they are using non-empty strings as cache keys.
This was an issue with update_user_caches()
and clean_user_cache()
, I've fixed this in https://github.com/WordPress/wordpress-develop/pull/2967/commits/c8f38c97b1f88263bf7249ad2d3bdabac18d60e7
This is likely causing issues right now and should be fixed in the next patch release.
1) Tests_User::test_wp_insert_user_with_meta Unexpected incorrect usage notice for add. Cache key must not be an empty string. (This message was added in version 6.1.0.) 2) Tests_User_GetUserCount::test_get_user_count_on_different_network Unexpected incorrect usage notice for add. Cache key must not be an empty string. (This message was added in version 6.1.0.) 3) Tests_User_GetUserCount::test_enable_live_network_user_counts_filter Unexpected incorrect usage notice for add. Cache key must not be an empty string. (This message was added in version 6.1.0.) 4) Tests_User_GetUserCount::test_get_user_count_update_on_delete_multisite Unexpected incorrect usage notice for add. Cache key must not be an empty string. (This message was added in version 6.1.0.) 5) Tests_User_Slashes::test_wp_insert_user Unexpected incorrect usage notice for add. Cache key must not be an empty string. (This message was added in version 6.1.0.)
#10
@
2 years ago
@SergeyBiryukov: An alternative to debug_backtrace()
could be appending wp_debug_backtrace_summary()
to the doing it wrong message. it would also make tracing these errors much easier. Thoughts?
#11
@
2 years ago
I'm against floats, as this will just introduce all kinds of typecasting issues with external caches then.
#13
follow-up:
↓ 14
@
2 years ago
I'm against floats, as this will just introduce all kinds of typecasting issues with external caches then.
Agreed.
What about the trimming of empty groups?
We can safeguard against that as well in the patch, if desired. I seem to recall that empty(trim())
doesn't work on PHP 5.6 and it may need a dedicated variable, which would mean more code.
#14
in reply to:
↑ 13
;
follow-up:
↓ 15
@
2 years ago
Replying to tillkruess:
What about the trimming of empty groups?
We can safeguard against that as well in the patch, if desired. I seem to recall that
empty(trim())
doesn't work on PHP 5.6 and it may need a dedicated variable, which would mean more code.
I tested with PHP 5.6.40 and could not reproduce such an issue. There is no extra variable needed: (same result for PHP 5.6, 7.4 and 8.1)
<?php $group = ' '; if ( empty( trim( $group ) ) ) { echo "empty"; }
#15
in reply to:
↑ 14
;
follow-up:
↓ 16
@
2 years ago
Replying to malthert:
I tested with PHP 5.6.40 and could not reproduce such an issue. There is no extra variable needed: (same result for PHP 5.6, 7.4 and 8.1)
I could swear this used to be a thing.
Anyhow, would it not make sense to check the group similar to is_valid_key()
, since "0"
is technically a valid group, but fail with empty(trim('0'))
?
#16
in reply to:
↑ 15
;
follow-up:
↓ 17
@
2 years ago
Definitely, is_valid_group() would replace the empty() for fallback to default group - with no error message for groups?
#17
in reply to:
↑ 16
@
2 years ago
Replying to malthert:
Definitely, is_valid_group() would replace the empty() for fallback to default group - with no error message for groups?
I'd also suggest a _doing_it_wrong()
as well, any objections?
SergeyBiryukov commented on PR #2967:
2 years ago
#18
I've spent some time taking a closer look at the update_user_caches()
/clean_user_cache()
changes.
Some observations:
- It is not possible to have an empty
user_login
oruser_nicename
:- If
user_login
is empty, the user is never saved to the database. - If
user_nicename
is empty, it is created from `user_login`.
- If
- So we only really need to check if
user_email
is not empty. I considered creating a ticket to disallow an empty email in the same way as an emptyuser_login
is disallowed, but it looks like creating a user with an empty email should actually be possible, see #14834. 0
or'0'
would not be a valid email, so I think aif ( ! empty( $user->user_email ) )
check would suffice here.- We have some tests where the email is passed to
wp_insert_user()
incorrectly or is accidentally reset, might as well fix that.
With that in mind, I've added a few commits to:
- Simplify the checks in
update_user_caches()
/clean_user_cache()
. - Correct some tests where the email is not passed as expected. This is not strictly required here, just some minor cleanup.
I think this looks good. Any feedback on these latest changes appreciated 🙂
tillkruss commented on PR #2967:
2 years ago
#19
I think this looks good. Any feedback on these latest changes appreciated 🙂
LGTM ✌️
#20
@
2 years ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 53818:
SergeyBiryukov commented on PR #2967:
2 years ago
#21
Thanks for the PR! Merged in r53818.
This ticket was mentioned in PR #3063 on WordPress/wordpress-develop by tillkruss.
2 years ago
#24
- Keywords has-unit-tests added
Follow-up to #2967 and a164d642759a6673ad832cdc3330856bb4084be8.
Trac ticket: https://core.trac.wordpress.org/ticket/56198
SergeyBiryukov commented on PR #3063:
2 years ago
#26
Thanks for the PR! Merged in r53834.
Trac ticket: https://core.trac.wordpress.org/ticket/56198