-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
[AssetMapper] Fix JavaScriptImportPathCompiler
regression in regex
#54079
[AssetMapper] Fix JavaScriptImportPathCompiler
regression in regex
#54079
Conversation
Hey! Thanks for your PR. You are targeting branch "6.4" but it seems your PR description refers to branch "6.4,". Cheers! Carsonbot |
I have also updated the https://regex101.com/r/1iBAIb/1 to https://regex101.com/r/1iBAIb/2 by adding the same change there. |
src/Symfony/Component/AssetMapper/Compiler/JavaScriptImportPathCompiler.php
Outdated
Show resolved
Hide resolved
Thanks for this :). Do we know if there is a test case that could be created to show the problem? Due to the complexity of this regex, we rely heavily on the tests to avoid regressions like this. |
@weaverryan The reproducer I created was in the linked issue #54078
|
If Im honest I dont really know what the root problem is enough to create some regression unit tests sorry. I just stumbled on and reported the issue first once I had a reproducer, and used @nicolas-grekas provided solution as a PR when requested. |
This happens because of big js files to parse with some content that trigger deep backtracking. Not easy to reliably test I fear... |
Thank you @PhilETaylor for this fix. Hopefully this one would bring us near the 99% we target, without having to implement a real JS parser (that would slow things down a lot for everyone) |
JavaScriptImportPathCompiler
regression in regex
I experienced the bug right now, while updating a branch i was working on the ux website... Fixed locally with a dirty patch... $relativeImportPath = $this->makeRelativeForJavaScript($relativeImportPath);
return str_replace($importedModule, $relativeImportPath, $fullImportString);
- }, $content, -1, $count, \PREG_OFFSET_CAPTURE);
+ }, $content, -1, $count, \PREG_OFFSET_CAPTURE) ?? $content;
} With your example i think i see the problem here ... https://regex101.com/r/WueQkU/1 ![]() .. and this file does not contains a single " So what about we also add a pre-check and ignore files that do not contain " |
@smnandre A Btw, I just thought about a case not covered by the current regex. But I'll wait for this PR to be merged before doing my PR as they would conflict. |
Of course! I just suggested an additional check :) I'll open another one to throw an expression instead of returning null. |
18f04a8
to
ae16b2d
Compare
Thank you @PhilETaylor. |
…PCRE error (smnandre) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [AssetMapper] Throw exception in Javascript compiler when PCRE error | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #... | License | MIT `preg_match_callback` can return null when a PCRE error occured, leading there to a TypeError. Let's throw an exception and expose the error message. (follows #54078 / complementary to #54079 ) Commits ------- 2333b58 [AssetMapper] Throw exception in Javascript compiler when PCRE error
Got the same problem with |
Fixes regression in regex, fix provided by @nicolas-grekas in #54078