Repository navigation
Autoload max size - #5671
Autoload max size#5671pbearne wants to merge 125 commits into
Conversation
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>
# Conflicts: # tests/phpunit/tests/option/option.php
test was failing
But this has broken the tests But that said the new result may be right as the sizz override the value passed in
…` and expand test coverage.
felixarntz
left a comment
There was a problem hiding this comment.
This looks great to me at this point. Thanks for all the iterations @pbearne!
joemcgill
left a comment
There was a problem hiding this comment.
I think this is looking great. Nice work @pbearne and @felixarntz. I've left a few comments, but nothing blocking. I think we can land this early and address new concerns as follow-up tasks.
Technically speaking, I do agree with @felixarntz's concern here, though I think it would be better to leave things as is for now, rather than creating the opportunity for unexpected values to be supported as "true" and wait for real bug reports to determine if we need to expand the checks in that switch statement.
| * @return string Returns the original $autoload value if explicit, or 'auto-on', 'auto-off', | ||
| * or 'auto' depending on default heuristics. | ||
| */ | ||
| function wp_determine_option_autoload_value( $option, $value, $serialized_value, $autoload ) { |
There was a problem hiding this comment.
Not a blocker, but I think the purpose of this function is not clear based on its name. I'd propose changing this to wp_normalize_option_autoload_value() instead. Happy to consider this in in a follow-up ticket if needed.
There was a problem hiding this comment.
It's an @access private function anyway, so we could change the name later. Happy to reconsider in a follow up.
| * @param string $option The passed option name. | ||
| * @param mixed $value The passed option value to be saved. | ||
| */ | ||
| $autoload = apply_filters( 'wp_default_autoload_value', null, $option, $value, $serialized_value ); |
There was a problem hiding this comment.
At this point, we're assuming the autoload value is undetermined (i.e., null), but that's not necessarily the case. That's probably fine, since we're already normalized and returned all of the supported values above, but could lead to unforeseen bug reports. I don't think we need to adjust anything, but wanted to flag for consideration.
|
@felixarntz I left feedback on your comment in my PR review. But since that doesn't show up in Trac, I'll summarize: Technically speaking, I agree but I think it would be better to leave things as is for now, rather than creating the opportunity for unexpected values to be supported as "true" and wait for real bug reports to determine if we need to handle other values. |
|
@joemcgill Replying to your feedback:
I would generally agree with your assessment, but leaving this BC value handling out actually breaks previously existing unit tests. That to me is a reasonable indicator that we need to have this condition to convert technically unsupported values to LMKWYT. |
|
Ah, I had missed that there were previous tests breaking. In that case, I’d prioritize ensuring previous tests still pass, though it’s probably worth reviewing why the tests were written the way they were to see if they are unnecessarily broad |
|
@joemcgill Looking at the source of those tests, they are quite old, coming from https://core.trac.wordpress.org/ticket/31119 / https://core.trac.wordpress.org/changeset/31278. Since at that time a boolean I don't feel strongly about the course of action here either way. I don't think there's realistically a great danger of changing those tests, because it's unlikely values like Concluding, I'd be okay removing the BC handling and adjusting the relevant tests to assert |
|
@felixarntz I think I'm missing something. Looking at this PR, the test cases you're referencing don't seem to be changed (besides changing |
|
@joemcgill That's my point. They aren't changed right now because the PR includes the extra BC handling. But if I remove that, they'll fail, so they'd need to be changed to expect |
felixarntz
left a comment
There was a problem hiding this comment.
@joemcgill Below are the changes that would be needed to remove the BC handling.
That's what I missed 😄 — I thought you were asking whether we should add some BC handling to this PR, not whether we should remove the BC handling that was already put in place. Thanks for clarifying. I actually think I prefer the changes you've suggested above to handle unsupported values as if they were |
…provements. Co-authored-by: Joe McGill <801097+joemcgill@users.noreply.github.com> Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
joemcgill
left a comment
There was a problem hiding this comment.
The latest updates look good to me, @felixarntz.
|
I actually tested the goal of this PR. My comments on the announcement post went ignored and bureaucratically buried. Yet the theoretical reasoning ("large autoload queries are slow") doesn't hold up under measurement. Here are the results of 100 options of set size being added, with and without the default threshold set, the options cache is then cleared, and then again autoloaded + individually queried: message.15.txt. Summary:
The reasoning is simple:
A lot of resources and time have been invested in discussing this and many related issues. It astounds me that no one has thought to benchmark this first. I'd have assumed that Core Performance, albeit spawned by individuals with zero experience in performant code, would've thought to benchmark changes before implementing them. |
Code refresh
Trac ticket: https://core.trac.wordpress.org/ticket/42441