Skip to content

Fix: CloneBlock regexp for different regexp engine with xml line-drop #1801

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

Closed
wants to merge 1 commit into from

Conversation

juzser
Copy link
Contributor

@juzser juzser commented Jan 7, 2020

Description

Reference Issue: #1800
When you have multiple Blocks in multiple Rows, some of blocks cannot be replaced
Only the last block was replaced.
This because some word file will drop-line after xml tag, and the regular expression engine in different versions cannot find the matched block.

Fixes # (issue)
File: TemplateProcessor.php
Line: 740

Change the regexp pattern to:

'/(.*((?s)<w:p\b(?:(?!<w:p\b).)*?\${' . $blockname . '}<\/w:.*?p>))(.*)((?s)<w:p\b(?:(?!<w:p\b).)*?\${\/' . $blockname . '}<\/w:.*?p>)/is',

Describe:

  • Do not find for xml.
  • Start find the <w:p tag that closest to the blockname.
  • This <w:p does not contain any other <w:p inside it.

Checklist:

All Done

  • 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

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.633% when pulling 5f28595 on JuzSer:cloneblock-regexp into 733f845 on PHPOffice:develop.

@juzser
Copy link
Contributor Author

juzser commented Jan 21, 2020

Update PR: #1809

@Herz3h
Copy link

Herz3h commented Feb 11, 2020

I encountered an I issue where my blocks were not cloned (basically preg_match didn't match anything). I changed regex to what's used here, and it was fixed. Can we merge this ?

@juzser
Copy link
Contributor Author

juzser commented Feb 14, 2020

I encountered an I issue where my blocks were not cloned (basically preg_match didn't match anything). I changed regex to what's used here, and it was fixed. Can we merge this ?

@Herz3h Feel free to use it. Or you can use my fork repo here: https://github.com/JuzSer/PHPWord

@Herz3h
Copy link

Herz3h commented Feb 17, 2020

Would prefer to have it merged, so I don't forget later on that I'm using your repo instead of the official one in my composer ;)

@xu-li
Copy link

xu-li commented Mar 24, 2020

In my case, it works if pcre.jit is enabled on php 7.3.6. And it will also work if pcre.backtrack-limit is big enough when pcre.jit. is disabled.

Those might not be good solutions, but could be served as temporary solution before the official fix.

@juzser
Copy link
Contributor Author

juzser commented Mar 24, 2020

In my case, it works if pcre.jit is enabled on php 7.3.6. And it will also work if pcre.backtrack-limit is big enough when pcre.jit. is disabled.

Those might not be good solutions, but could be served as temporary solution before the official fix.

@xu-li Yes, I see that problem before, long time ago I posted an update PR here: #1809
This will solve the catastrophic backtracking. You can try this.

@xu-li
Copy link

xu-li commented Mar 25, 2020

Thanks, @juzser . Yes, I saw your PR, tried that and it worked in my particular case.

I didn't use your code because:

  • We use phpword in production.
  • Regular expression is a beast to me and I am not familiar with the details of cloneBlock. I couldn't evaluate if your regular expression will work for all the use cases.

It's just my personal preference. I want to play on the safe side. It has nothing to do with your code.

@juzser
Copy link
Contributor Author

juzser commented Mar 25, 2020

Yes, of course. I've tested my regexp with some cases, I think it safe but yes, you're correct, maybe I missed something that I don't know.

I'm just waiting for the package's owner to check this issue.. But seem likes he has no longer maintenance this package, maybe.

@troosan
Copy link
Contributor

troosan commented Feb 7, 2021

merged #1809

@troosan troosan closed this Feb 7, 2021
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.

5 participants