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

db search: fix match in context in non-regex case. #45

Merged
merged 2 commits into from Aug 30, 2017

Conversation

2 participants
@gitlost
Contributor

gitlost commented Aug 29, 2017

Issue #43

Removes context regex and always uses PREG_OFFSET_CAPTURE mode in preg_match_all() , to avoid missing matches in contexts.

Also no longer adds UTF-8 modifier u to regex ever, as not needed without context regex and was never needed anyway in --regex case.

Also rechecks encoding when non-UTF-8 for each value of $col_val as could get ASCII first time (unfortunately behat/gherkin can't handle non-UTF-8 chars in a feature file so will need a phpunit test to test this properly).

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

$offset = $match_arr[1];
// Offsets are in bytes, so need to use `strlen()` and `substr()` before using `safe_substr()`.
$before = $before_context && $offset ? \cli\safe_substr( substr( $col_val, 0, $offset ), -$before_context, null /*length*/, false /*is_width*/, $col_encoding ) : '';
$after = $after_context ? \cli\safe_substr( substr( $col_val, $offset + strlen( $match ) ), 0, $after_context, false /*is_width*/, $col_encoding ) : '';

This comment has been minimized.

@schlessera

schlessera Aug 30, 2017

Member

Just a general tip for code readability:

If you have scalar arguments where it is not immediately clear what their meaning is, you can use a simple assignment to make it clearer.

So, instead of null /*length*/, false /*is_width*/, you could also do $length = null, $is_width = false.

I personally find this easier to read.

@schlessera

schlessera Aug 30, 2017

Member

Just a general tip for code readability:

If you have scalar arguments where it is not immediately clear what their meaning is, you can use a simple assignment to make it clearer.

So, instead of null /*length*/, false /*is_width*/, you could also do $length = null, $is_width = false.

I personally find this easier to read.

This comment has been minimized.

@gitlost

gitlost Aug 30, 2017

Contributor

Yes, I agree, it is easier to read - however, gasp horror it's an unnecessary assignment! Knowingly doing an unnecessary assignment is the sort of thing that keeps me from sleeping! So I'll leave it for the mo...

@gitlost

gitlost Aug 30, 2017

Contributor

Yes, I agree, it is easier to read - however, gasp horror it's an unnecessary assignment! Knowingly doing an unnecessary assignment is the sort of thing that keeps me from sleeping! So I'll leave it for the mo...

@schlessera

This comment has been minimized.

Show comment
Hide comment
@schlessera

schlessera Aug 30, 2017

Member

@gitlost Please merge in latest master.

Member

schlessera commented Aug 30, 2017

@gitlost Please merge in latest master.

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

1 check passed

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

@gitlost gitlost deleted the context_bug branch Aug 30, 2017

@gitlost gitlost added this to the 1.2.1 milestone Aug 30, 2017

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