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

Handle PCRE errors gracefully #75

Merged
merged 2 commits into from Apr 20, 2018

Conversation

2 participants
@schlessera
Member

schlessera commented Apr 19, 2018

We leave the content untouched (instead of replacing with null) and issue a warning.

This is far from ideal, as it can potentially spam the screen full of warnings. However, a better mechanism will require a total refactor of the SearchReplacer class.

Fixes #65

Handle PCRE errors gracefully
We leave the content untouched (instead of replacing with `null`) and issue a warning.

This is far from ideal, as it can potentially spam the screen full of warnings. However, a better mechanism will require a total refactor of the `SearchReplacer` class.

Fixes #65

@schlessera schlessera added this to the 1.3.0 milestone Apr 19, 2018

@schlessera schlessera requested a review from wp-cli/committers Apr 19, 2018

@schlessera

This comment has been minimized.

Member

schlessera commented Apr 19, 2018

I fail to find a working test to provoke a PCRE error (without just brute-forcing a stack overflow).

If anyone has a good idea or some sample code of how to provoke a runtime PCRE check, please let me know so I can add a regression test here.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Apr 19, 2018

@schlessera Looks like tests need to be updated for trunk here too?

@schlessera

This comment has been minimized.

Member

schlessera commented Apr 19, 2018

@danielbachhuber danielbachhuber merged commit b45eeb4 into master Apr 20, 2018

1 check passed

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

@danielbachhuber danielbachhuber deleted the 65-check-for-pcre-error-on-return branch Apr 20, 2018

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