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

Improve validation for `--regex-flags` and `--regex-delimiter` and add tests for them #30

Merged
merged 8 commits into from Aug 30, 2017

Conversation

4 participants
@miya0001
Member

miya0001 commented Aug 29, 2017

If regex is /, regex will be escaped like following.

http:\/\/example.com
@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 29, 2017

Contributor

I think it should be left up to the user to escape the delimiters. Trying to manipulate the passed-in regex is guaranteed misery! It should though be mentioned in the docblock and an example with escaping given, cf db search.

Contributor

gitlost commented Aug 29, 2017

I think it should be left up to the user to escape the delimiters. Trying to manipulate the passed-in regex is guaranteed misery! It should though be mentioned in the docblock and an example with escaping given, cf db search.

@miya0001 miya0001 changed the title from escape the regex delimiter string in regex to Improve validation for `--regex-flags` and `--regex-delimiter` and add tests for them Aug 29, 2017

@miya0001 miya0001 requested review from gitlost and wp-cli/committers Aug 29, 2017

@miya0001

This comment has been minimized.

Show comment
Hide comment
@miya0001

miya0001 Aug 29, 2017

Member

@gitlost This check is nice idea! 😮

if ( false === @preg_match( $search_regex, '' ) ) {
    \WP_CLI::error( "The regex '$search_regex' fails." );
}
Member

miya0001 commented Aug 29, 2017

@gitlost This check is nice idea! 😮

if ( false === @preg_match( $search_regex, '' ) ) {
    \WP_CLI::error( "The regex '$search_regex' fails." );
}
@@ -84,7 +84,14 @@ private function _run( $data, $serialised, $recursion_level = 0, $visited_data =
else if ( is_string( $data ) ) {
if ( $this->regex ) {
$data = preg_replace( "$this->regex_delimiter$this->from$this->regex_delimiter$this->regex_flags", $this->to, $data );
$search_regex = $this->regex_delimiter;

This comment has been minimized.

@gitlost

gitlost Aug 29, 2017

Contributor

Could you move this check to the top level where you had the previous check so it only gets run once?

@gitlost

gitlost Aug 29, 2017

Contributor

Could you move this check to the top level where you had the previous check so it only gets run once?

This comment has been minimized.

@miya0001

miya0001 Aug 30, 2017

Member

It is moved:

if ( ! empty( $this->regex ) ) {
$search_regex = $this->regex_delimiter;
$search_regex .= $old;
$search_regex .= $this->regex_delimiter;
$search_regex .= $this->regex_flags;
if ( false === @preg_match( $search_regex, '' ) ) {
\WP_CLI::error( "The regex '$search_regex' fails." );
}
}

@miya0001

miya0001 Aug 30, 2017

Member

It is moved:

if ( ! empty( $this->regex ) ) {
$search_regex = $this->regex_delimiter;
$search_regex .= $old;
$search_regex .= $this->regex_delimiter;
$search_regex .= $this->regex_flags;
if ( false === @preg_match( $search_regex, '' ) ) {
\WP_CLI::error( "The regex '$search_regex' fails." );
}
}

@gitlost

This comment has been minimized.

Show comment
Hide comment
@gitlost

gitlost Aug 29, 2017

Contributor

This check is nice idea!

Yes, I forget where I stole it from!

Contributor

gitlost commented Aug 29, 2017

This check is nice idea!

Yes, I forget where I stole it from!

miya0001 added some commits Aug 30, 2017

@miya0001

This comment has been minimized.

Show comment
Hide comment
@miya0001

miya0001 Aug 30, 2017

Member

It is ready. :)

Member

miya0001 commented Aug 30, 2017

It is ready. :)

miya0001 added some commits Aug 30, 2017

@gitlost gitlost merged commit fdc8b84 into master Aug 30, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gitlost gitlost deleted the escape-regex branch Aug 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment