Add --regex-limit option. #62

miya0001 · 2018-01-27T06:28:11Z

Related: #61

miya0001 · 2018-01-27T09:34:26Z

src/Search_Replace_Command.php

 	private $log_prefixes = array( '< ', '> ' );
 	private $log_colors;
 	private $log_encoding;
-	private $log_run_data = array();


It is an unused variable.

gitlost · 2018-02-11T18:51:09Z

features/search-replace.feature

      """
      world, Hello
      """
+    When I run `wp search-replace 'o' 'O' --regex --regex-limit=1`


I think --regex-limit deserves its own scenario(s) with targetted data rather than piggy-backing on existing ones.

gitlost · 2018-02-11T18:51:33Z

features/search-replace.feature

      kppr
      """
    And the return code should be 1
+    When I run `wp search-replace 'o' 'O' --regex --regex-limit=1`


gitlost · 2018-02-11T18:53:57Z

features/search-replace.feature

      Content_ab\1z__baz_1234567890_eb\1z__bez_1234567890_ib\1z__biz_1234567890_ob\1z__boz_1234567890_ub\1z__buz_
      """

+    When I run `wp search-replace '_{2}' '-' wp_posts --regex --regex-limit=2 --log --before_context=11 --after_context=11`


gitlost · 2018-02-11T18:57:18Z

src/Search_Replace_Command.php

 	 * : The delimiter to use for the regex. It must be escaped if it appears in the search string. The default value is the result of `chr(1)`.
 	 *
+	 * [--regex-limit=<regex-limit>]
+	 * : The maximum possible replacements for each pattern in each subject string. Defaults to `-1` (no limit).


I see you copied this from http://php.net/manual/en/function.preg-replace.php but it needs adapting here, eg there's only one subject string. Maybe The maximum possible replacements for the regex per row (or per unserialized data bit per row). Defaults... or something (need to mention the special serialized data case I think).

gitlost · 2018-02-11T19:00:29Z

src/Search_Replace_Command.php

 		$report          = array();
 		$this->dry_run         = \WP_CLI\Utils\get_flag_value( $assoc_args, 'dry-run' );
-		$php_only        = \WP_CLI\Utils\get_flag_value( $assoc_args, 'precise' );
+		$php_only              = \WP_CLI\Utils\get_flag_value( $assoc_args, 'precise' );


Not a fan of manually aligning assignments that happen to follow each other (it's so awkward to maintain and fundamentally random - when do you start/stop?!) so personally would just remove but ignore me.

gitlost · 2018-02-11T19:25:22Z

src/Search_Replace_Command.php

 		$old_bits = $new_bits = array();
 		$append_next = false;
 		$last_old_offset = $last_new_offset = 0;
 		$match_cnt = count( $old_matches[0] );


I think the easiest thing is just to do

$match_cnt = count( $new_matches[0] ); // `$new_matches[0]` may be limited by `regex_limit` so use instead of `$old_matches[0]`.

here which means the empty() checks following aren't needed.

@gitlost why not use the $count preg_replace_callback provides?

Er, well one could, but it's not important and I don't think reviewing a review is going to work to be honest! If you'd like to contribute please create an idea or an issue or a PR.

On second thoughts you're right @alpipego that is a nicer way to do it so as suggested in #70 @miya0001 you can just do }, $old_data, $this->regex_limit, $match_cnt ); at line 811 instead.

don't think reviewing a review is going to work

Of course you're right. The context is, that I am going through this with @miya0001 at my side at WordCamp Bangkok Contributer Day with the goal of fixing his PR.

Ah I see! Sorry, I thought the PR was being hi-jacked! Ignore my previous comments!

gitlost · 2018-02-11T19:26:13Z

src/Search_Replace_Command.php

 		$last_old_offset = $last_new_offset = 0;
 		$match_cnt = count( $old_matches[0] );
 		for ( $i = 0; $i < $match_cnt; $i++ ) {
+			if ( empty( $old_matches[0][ $i ] ) || empty( $new_matches[0][ $i ] ) ) {


So this change isn't needed.

gitlost · 2018-02-11T19:26:30Z

src/Search_Replace_Command.php

-					$new_after = substr( $new_after, 0, $new_matches[0][ $i + 1 ][1] - $new_end_offset );
-					$after_shortened = true;
-					// On the next iteration, will append with no before context.
+					if ( ! empty( $old_matches[0][ $i + 1 ] ) && ! empty( $new_matches[0][ $i + 1 ] ) ) {


Nor this change.

gitlost · 2018-02-11T19:29:42Z

src/Search_Replace_Command.php

 		$this->format          = \WP_CLI\Utils\get_flag_value( $assoc_args, 'format' );
+		$this->regex_limit    = \WP_CLI\Utils\get_flag_value( $assoc_args, 'regex-limit', -1 );
+		if ( 0 === intval( $this->regex_limit ) ) {
+			WP_CLI::error( '`--regex-limit` expects integer.' );


Need a test for this anyway.

gitlost · 2018-02-11T19:33:08Z

src/Search_Replace_Command.php

 		);

-		$replacer = new \WP_CLI\SearchReplacer( $old, $new, $this->recurse_objects, $this->regex, $this->regex_flags, $this->regex_delimiter );
+		$replacer = new \WP_CLI\SearchReplacer( $old, $new, $this->recurse_objects, $this->regex, $this->regex_flags, $this->regex_delimiter, $this->regex_limit );


gitlost · 2018-02-11T19:40:00Z

Not sure why the review comments are listed in a weird order...?!

gitlost · 2018-02-17T07:46:51Z

Closing this in favor of #70

gitlost · 2018-02-17T07:51:22Z

Re #70 (comment) okay I'll leave both open and step away for a while!

Patch 61

miya0001 · 2018-02-18T04:30:32Z

@gitlost

Thanks for review.
I merged #70 into this PR for now and I am going to work to improve this patch.
Please wait a couple of days. 😊

alpipego

@miya0001 the default should of course be -1 (and not 1).

alpipego · 2018-02-19T06:27:07Z

src/Search_Replace_Command.php

 	private $regex;
 	private $regex_flags;
 	private $regex_delimiter;
+	private $regex_limit = 1;


@miya0001 the default should of course be -1 (instead of 1).

thanks 👍

miya0001 · 2018-03-08T20:02:58Z

@gitlost
Please review.

gitlost

Thanks @miya0001 , just some extra tests needed and some fiddling with the arg checking and a few doc and indentation nitpicks.

gitlost · 2018-03-08T21:57:46Z

features/search-replace.feature

+
+    When I run `wp search-replace --regex "ap{2}le" "orange" --regex-limit=1 --log`
+    Then STDOUT should contain:
+    """


Could you indent the PyString by 2 spaces here please (to match standard indentation).

gitlost · 2018-03-08T21:58:48Z

features/search-replace.feature

+    I have a pen, I have an orange. Pen, pine-apple, apple-pen.
+    """
+
+


And remove the extra newline here,

gitlost · 2018-03-08T21:59:00Z

features/search-replace.feature

+
+    When I run `wp search-replace --regex "ap{2}le" "orange" --regex-limit=2 --log`
+    Then STDOUT should contain:
+    """


Again indent by standard 2 spaces please.

gitlost · 2018-03-08T22:30:11Z

features/search-replace.feature

+    Then STDOUT should contain:
+    """
+    I have a pen, I have an orange. Pen, pine-orange, apple-pen.
+    """


Could you add a scenario to test for incorrect args here, eg

Scenario: Regex search/replace with incorrect or default `--regex-limit` Given a WP install When I try `wp search-replace '(Hello)\s(world)' '$2, $1' --regex --regex-limit=asdf` Then STDERR should be: """ Error: `--regex-limit` expects a non-zero positive integer or -1. """ When I try `wp search-replace '(Hello)\s(world)' '$2, $1' --regex --regex-limit=0` Then STDERR should be: """ Error: `--regex-limit` expects a non-zero positive integer or -1. """ When I try `wp search-replace '(Hello)\s(world)' '$2, $1' --regex --regex-limit=-2` Then STDERR should be: """ Error: `--regex-limit` expects a non-zero positive integer or -1. """ When I run `wp search-replace '(Hello)\s(world)' '$2, $1' --regex --regex-limit=-1` Then STDOUT should contain: """ Success: """

(See below for disallowing zero.)

gitlost · 2018-03-08T22:32:54Z

src/Search_Replace_Command.php

 	 * : The delimiter to use for the regex. It must be escaped if it appears in the search string. The default value is the result of `chr(1)`.
 	 *
+	 * [--regex-limit=<regex-limit>]
+	 * : The maximum possible replacements for the regex in each unserialized data bit per row. Defaults to `-1` (no limit).


The serialized case is one case only and secondary, so I think The maximum possible replacements for the regex per row (or per unserialized data bit per row). Defaults to -1 (no limit). is better.

gitlost · 2018-03-08T22:37:54Z

src/Search_Replace_Command.php

 				}
 				WP_CLI::error( $msg );
 			}
+			if ( ( $regex_limit = (int) \WP_CLI\Utils\get_flag_value( $assoc_args, 'regex-limit', - 1 ) ) > - 1 ) {


This check should be moved to the previous block where the other options are checked (after line 183) and should not allow zero as it doesn't make sense to me. So I'd prefer:

if ( null !== ( $regex_limit = \WP_CLI\Utils\get_flag_value( $assoc_args, 'regex-limit' ) ) ) { if ( ! preg_match( '/^(?:[0-9]+|-1)$/', $regex_limit ) || 0 === (int) $regex_limit ) { WP_CLI::error( '`--regex-limit` expects a non-zero positive integer or -1.' ); } $this->regex_limit = (int) $regex_limit; }

gitlost · 2018-03-08T22:38:48Z

src/Search_Replace_Command.php

 				if ( $i + 1 < $match_cnt && $old_end_offset + strlen( $old_after ) > $old_matches[0][ $i + 1 ][1] ) {
-					$old_after = substr( $old_after, 0, $old_matches[0][ $i + 1 ][1] - $old_end_offset );
-					$new_after = substr( $new_after, 0, $new_matches[0][ $i + 1 ][1] - $new_end_offset );
+					$old_after       = substr( $old_after, 0, $old_matches[0][ $i + 1 ][1] - $old_end_offset );


These 2 inner indentation changes are unnecessary

gitlost · 2018-03-08T22:39:41Z

src/WP_CLI/SearchReplacer.php

 	 * @param bool    $regex           Whether `$from` is a regular expression.
 	 * @param string  $regex_flags     Flags for regular expression.
 	 * @param string  $regex_delimiter Delimiter for regular expression.
+	 * @param integer $regex_limit     The maximum possible replacements for each pattern in each subject string.


This should appear last after the $logging arg now.

miya0001 · 2018-06-14T10:50:53Z

@schlessera

I forgot to work this ticket. You can review it now. :)

schlessera · 2018-06-18T09:57:48Z

Thanks for the PR, @miya0001 !

schlessera · 2018-06-18T12:12:43Z

Also thanks to @alpipego for the collaboration, as his original PR is included in this as well.

Add --regex-limit option.

miya0001 added 7 commits January 27, 2018 15:04

add param regex_limit

eb22696

add param value to SearchReplacer()

262ca11

add regex_limit to SearchReplacer()

bbc8e41

remove unused line

a8accb0

rmoved unused var $log_run_data

7baef64

add features for --regex-limit options

244220b

fix problem for features

c69cc48

miya0001 commented Jan 27, 2018

View reviewed changes

miya0001 requested a review from a team January 27, 2018 09:34

miya0001 added 2 commits January 27, 2018 21:05

add features for --log and --regex-limit

e9d617a

error if string given for --regex-limit

4682304

gitlost suggested changes Feb 11, 2018

View reviewed changes

gitlost added the command:search-replace label Feb 11, 2018

gitlost added this to the 1.3.0 milestone Feb 11, 2018

gitlost mentioned this pull request Feb 11, 2018

Refactor WP_CLI\SearchReplacer functionality. #67

Open

alpipego added 6 commits February 17, 2018 12:48

reverse order of arguments, to keep bc

fb17561

add default value for regex_limit

e559ecf

change option description as per comment

cb0cbf3

validate and assign regex_limit

abc8f4c

use preg_replace count

77adaac

revert unneeded changes

8038f09

gitlost mentioned this pull request Feb 17, 2018

Improve --regex-limit logic #70

Merged

gitlost closed this Feb 17, 2018

gitlost reopened this Feb 17, 2018

miya0001 added 3 commits February 18, 2018 13:06

Merge pull request #70 from alpipego/patch-61

f542ed4

Patch 61

Merge branch 'master' into patch-61

5b33953

improve tests for --regex-limit

b49eb56

alpipego suggested changes Feb 19, 2018

View reviewed changes

miya0001 added 2 commits March 7, 2018 00:57

Merge branch 'master' into patch-61

8c471e9

set default value of the regex limit to -1

da44f6a

gitlost suggested changes Mar 8, 2018

View reviewed changes

schlessera removed this from the 1.3.0 milestone Apr 21, 2018

miya0001 added 3 commits April 28, 2018 14:44

solve conflicts

e5d16b7

Merge branch 'master' into patch-61

1751673

some fix

c52fd0f

miya0001 requested a review from schlessera June 14, 2018 10:50

schlessera approved these changes Jun 18, 2018

View reviewed changes

schlessera added this to the 1.4.0 milestone Jun 18, 2018

schlessera merged commit 9947e9e into master Jun 18, 2018

schlessera deleted the patch-61 branch June 18, 2018 09:57

CodeProKid mentioned this pull request Jul 20, 2018

Add --regex-limit #61

Closed

danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022

Merge pull request #62 from wp-cli/patch-61

0045d92

Add --regex-limit option.

Copilot AI mentioned this pull request Feb 1, 2026

Refactor SearchReplacer into Search_Replace_Command #212

Open

11 tasks

		I have a pen, I have an orange. Pen, pine-apple, apple-pen.
		"""

Add --regex-limit option. #62

Add --regex-limit option. #62

Uh oh!

Conversation

miya0001 commented Jan 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gitlost commented Feb 11, 2018

Uh oh!

gitlost commented Feb 17, 2018

Uh oh!

gitlost commented Feb 17, 2018

Uh oh!

miya0001 commented Feb 18, 2018

Uh oh!

alpipego left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miya0001 commented Mar 8, 2018

Uh oh!

gitlost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miya0001 commented Jun 14, 2018

Uh oh!

schlessera commented Jun 18, 2018

Uh oh!

schlessera commented Jun 18, 2018

alpipego left a comment •

edited

Loading