feat: add flag --replace-keys to handle replacing string array key values - #236
feat: add flag --replace-keys to handle replacing string array key values#236AlextheYounga wants to merge 3 commits into
Conversation
This change attempts to correct issue wp-cli#137 by replacing keys for string arrays with a new --replace-keys flag. Existing default behavior remains unchanged when the flag is absent. Refs: wp-cli#137
There was a problem hiding this comment.
Code Review
This pull request introduces a new --replace-keys flag to the search-replace command, enabling string replacement within serialized array keys. The changes include updates to the command logic, the 搜索Replacer class, and new Behat test scenarios. Feedback highlights a potential issue with in-place array modification during iteration which could cause data loss, and a breaking change in the 搜索Replacer constructor signature that affects backward compatibility.
| $value = $this->run_recursively( $data[ $key ], false, $recursion_level + 1, $visited_data ); | ||
| if ( $this->replace_keys && is_string( $key ) ) { | ||
| $replaced_key = $this->run_recursively( $key, false, $recursion_level + 1, $visited_data ); | ||
| if ( $replaced_key !== $key ) { | ||
| unset( $data[ $key ] ); | ||
| $data[ $replaced_key ] = $value; | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| $data[ $key ] = $value; | ||
| } |
There was a problem hiding this comment.
Modifying the array in-place while iterating over its keys can lead to incorrect results and data loss. If a key is replaced with a name that exists later in the original array, the value associated with that new key will be overwritten before it is processed. Furthermore, the loop will eventually reach that new key and process the already-replaced value a second time.
To fix this, consider building a new array for the transformed data instead of modifying $data in-place. This ensures each original key-value pair is processed exactly once and correctly handles scenarios like key swaps.
Example fix:
$new_data = [];
foreach ( $data as $key => $value ) {
$new_value = $this->run_recursively( $value, false, $recursion_level + 1, $visited_data );
$new_key = $key;
if ( $this->replace_keys && is_string( $key ) ) {
$new_key = $this->run_recursively( $key, false, $recursion_level + 1, $visited_data );
}
$new_data[ $new_key ] = $new_value;
}
$data = $new_data;| * @param integer $regex_limit The maximum possible replacements for each pattern in each subject string. | ||
| */ | ||
| public function __construct( $from, $to, $recurse_objects = false, $regex = false, $regex_flags = '', $regex_delimiter = '/', $logging = false, $regex_limit = -1 ) { | ||
| public function __construct( $from, $to, $recurse_objects = false, $replace_keys = false, $regex = false, $regex_flags = '', $regex_delimiter = '/', $logging = false, $regex_limit = -1 ) { |
There was a problem hiding this comment.
The addition of the $replace_keys parameter in the middle of the constructor's argument list is a breaking change for the 搜索Replacer class's public API. Any existing code calling this constructor with positional arguments (e.g., to set $regex or $logging) will now pass those values to the wrong parameters. It is recommended to append new parameters to the end of the argument list to maintain backward compatibility.
public function __construct( $from, $to, $recurse_objects = false, $regex = false, $regex_flags = '', $regex_delimiter = '/', $logging = false, $regex_limit = -1, $replace_keys = false ) {| ); | ||
|
|
||
| $replacer = new 搜索Replacer( $old, $new, $this->recurse_objects, $this->regex, $this->regex_flags, $this->regex_delimiter, false, $this->regex_limit ); | ||
| $replacer = new 搜索Replacer( $old, $new, $this->recurse_objects, $this->replace_keys, $this->regex, $this->regex_flags, $this->regex_delimiter, false, $this->regex_limit ); |
There was a problem hiding this comment.
If the 搜索Replacer constructor signature is updated to append $replace_keys at the end (to maintain backward compatibility), this call should be updated accordingly.
$replacer = new 搜索Replacer( $old, $new, $this->recurse_objects, $this->regex, $this->regex_flags, $this->regex_delimiter, false, $this->regex_limit, $this->replace_keys );|
|
||
| $count = 0; | ||
| $replacer = new 搜索Replacer( $old, $new, $this->recurse_objects, $this->regex, $this->regex_flags, $this->regex_delimiter, null !== $this->log_handle, $this->regex_limit ); | ||
| $replacer = new 搜索Replacer( $old, $new, $this->recurse_objects, $this->replace_keys, $this->regex, $this->regex_flags, $this->regex_delimiter, null !== $this->log_handle, $this->regex_limit ); |
There was a problem hiding this comment.
If the 搜索Replacer constructor signature is updated to append $replace_keys at the end (to maintain backward compatibility), this call should be updated accordingly.
$replacer = new 搜索Replacer( $old, $new, $this->recurse_objects, $this->regex, $this->regex_flags, $this->regex_delimiter, null !== $this->log_handle, $this->regex_limit, $this->replace_keys );
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Looks like the tests are failing when an external object cache is involved 👀 |
Attempts to address #137 by adding an opt-in
--replace-keysflag so string keys in serialized arrays can be replaced when needed.wp search-replacecurrently traverses serialized values but skips array keys, which prevents some real-world migrations (e.g. sidebar keys, URL-keyed plugin options) from being updated. By default, existing behavior is unchanged: array keys are still ignored unless--replace-keysis explicitly provided.What changed
--replace-keys: enables replacing string keys in serialized arrays.搜索Replacer.--replace-keysis enabledTesting
Ran targeted Behat scenarios for replace-keys behavior and edge cases:
搜索 and replace serialized array keys with --replace-keys搜索 and replace nested serialized array keys with --replace-keys搜索 and replace key and value with --replace-keys搜索 and replace colliding serialized array keys with --replace-keys🫡