Skip to content
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

refactor: add wrapper around preg_match #2319

Closed
wants to merge 2 commits into from
Closed

refactor: add wrapper around preg_match #2319

wants to merge 2 commits into from

Conversation

drupol
Copy link

@drupol drupol commented Oct 26, 2022

Hi,

Description

The default behavior of preg_match is to silent any parsing error.
Therefore, I created a this wrapper is here to make sure that it properly throws an exception in case of any preg_match issue.

The origin of this PR comes from an issue we had at work. We were trying to delete a block in a .docx file (75Kb) and nothing was happening, despite the fact that the method issued correctly.
We made an introspection of the document.xml and discovered that the method TemplateProcessor::deleteBlock doesn't work, it seems that the regex is not valid (there are already a couple of issues about that: #2051, #2221, #2309, #2157), we are now using TemplateProcessor::cloneblock instead.
Then, we narrowed down the issue to the preg_match function and succeeded to reproduce the issue with a simple xml document.
This is were I discovered the function preg_last_error and noticed that the issue was related to the pcre.backtrack_limit PHP directive. Once we updated that directive from 1000000 to 2000000 it worked.

We lost less or more half a day on this issue, and this PR should help some people to see clearer when something goes wrong.

Could you please let me know if this PR is legit and if I should invest more time in it?

Nice to have: ideally, it would be a good think to invest time into an external contrib library that does preg_match correctly with the proper Exception handling.

Thanks

Checklist:

  • I have run composer run-script check --timeout=0 and no errors were reported
  • The new code is covered by unit tests (check build/coverage for coverage report)
  • I have updated the documentation to describe the changes

@PowerKiKi PowerKiKi changed the base branch from develop to master November 16, 2022 21:06
@drupol
Copy link
Author

drupol commented Feb 2, 2023

Closing this PR since no feedback after 3 months.

@drupol drupol closed this Feb 2, 2023
@drupol drupol deleted the add-preg_match-wrapper branch February 2, 2023 19:15
@kovalovme
Copy link

BUMP

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

Successfully merging this pull request may close these issues.

None yet

2 participants