-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Skip search and replace on objects that can't deserialize safely #192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @MarkBerube !
Can you include a Behat test for this change? Here's an introduction: https://make.wordpress.org/cli/handbook/contributions/pull-requests/#functional-tests
Confirmed that the unknown |
…adding behat tests
Behat scenario has been added! During behat testing it looks like I'm not entirely skipping that instance of search & replace if it ends up catching a |
Hm, I'm not sure this is quite correct. I think we should skip search/replace entirely if we can't deserialize the object. If we search/replace against a serialized string, we can break the serialization. |
Co-authored-by: Daniel Bachhuber <daniel@bachhuber.co>
I've made it so that an exception is thrown if you hit a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple small things. Looks like it's in pretty good shape otherwise.
src/WP_CLI/SearchReplacer.php
Outdated
@@ -107,7 +119,7 @@ private function run_recursively( $data, $serialised, $recursion_level = 0, $vis | |||
) | |||
); | |||
} else { | |||
foreach ( $data as $key => $value ) { | |||
foreach ( (array) $data as $key => $value ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it will have negative unintended consequences and should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no worries, removed with the latest commit!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it did lead to these incompatibilities that I don't have a clear fix for yet: https://github.com/wp-cli/search-replace-command/actions/runs/7053576340/job/19201047750 in these specifically failing PHP versions the sql object ends up like this:
object(mysqli_result)#1 (0) {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem here is that foreach
ing directly over an object is not always safe in PHP and these objects like above show why. Objects in PHP don't always allow iteration and functions like is_iterable()
check for the instance of \Traversable
on objects & aren't exactly reliable.
So instead of casting, I've found this to be the safest way to iterate through these objects:
try {
foreach ( $data as $key => $value ) {
$data->$key = $this->run_recursively( $value, false, $recursion_level + 1, $visited_data );
}
} catch ( \Error $e ) {
\WP_CLI::warning(
sprintf(
'Skipping an inconvertible serialized object: "%s", replacements might not be complete.',
$data
)
);
throw new Exception();
...
currently works in all versions of PHP I've tested, but I'll need to change my behat tests to account for specific php versions reporting this as a warning instead of a fatal error.
edit: actually just due to being warnings, using the try{} block seems successful in PHP 7.2, 7.4, 8.0 with these changes.
Thanks again for your work on this, @MarkBerube ! This PR is directionally correct. @schlessera is going to do a deep dive in the coming days to validate the approach, evaluate the nature of the PHP changes, etc. Once he approves, this change will be a candidate for a point release. |
No problem @danielbachhuber! Some of the behat tests provide a different warning in the older PHP versions. I couldn't get behat to conditionally behave with those warnings. Should I keep trying to fix them? cc @schlessera |
@MarkBerube No, don't spend more time on it until I've played around with this more. I'll ping here once I have an update. |
b6c4152
to
da1acc7
Compare
Thanks for all your work on this, @MarkBerube ! |
Of course @schlessera , happy to help out! |
As discussed in the following issue thread: #191
PHP 8.1 made it so class objects deserialized from strings must follow the types defined on the class if it has been loaded prior. Else you will face a PHP fatal TypeError:
This approach is to
catch
thoseTypeError
s when they happen, warn the user about it but then continue the S&R process. The only con to this approach that I can see is thatTypeError
according to PHP's own docs doesn't exist in PHP 5.6 but running the following test in PHP 5.6:shows no issues. Probably because PHP 5.6 isn't reaching the
catch()
block here.