Autoload max size #5671
Autoload max size #5671
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
Adjusted the formatting for better readability in the 'wpAutoloadValuesToAutoload.php' file. This enhancement applies to all test functions. Additionally, a commented-out filter has been removed from the 'option.php' file because it was not being used. This commit makes the code cleaner and improves the effectiveness of the tests.
…` and expand test coverage.
There was a problem hiding this comment.
This looks great to me at this point. Thanks for all the iterations @pbearne!
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 |
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>
|
Committed in https://core.trac.wordpress.org/changeset/57920 |
Code refresh
Trac ticket: https://core.trac.wordpress.org/ticket/42441