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

[8.x] Fix blade compiler regex issue #36843

Merged
merged 2 commits into from
Apr 1, 2021
Merged

[8.x] Fix blade compiler regex issue #36843

merged 2 commits into from
Apr 1, 2021

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Apr 1, 2021

Draft PR with failing test for a current issue with Blade components where a single ) is passed as a string argument. I've tracked it down to the regex below that doesn't seems to quite parse things correctly. Every attempt I made so far to fix it broke something else in the compiler.

'/\B@(@?\w+(?:::\w+)?)([ \t]*)(\( ( (?>[^()]+) | (?3) )* \))?/x', function ($match) {

Update: thanks to @jasonmccreary for fixing this 🥳

Fixes #36815

@driesvints driesvints changed the title [8.x] Add failing test [8.x] Fix blade compiler regex issue Apr 1, 2021
@driesvints driesvints marked this pull request as ready for review April 1, 2021 13:39
@driesvints driesvints requested a review from taylorotwell April 1, 2021 13:39
@taylorotwell taylorotwell merged commit b92d7b9 into 8.x Apr 1, 2021
@taylorotwell taylorotwell deleted the fix-blade-components branch April 1, 2021 13:51
@taylorotwell
Copy link
Member

I'm scared but we'll give it a shot. 😅

@jasonmccreary
Copy link
Contributor

Agreed. The idea is to handle any quoted strings separately. This is a bit naive to all the various ways one may quote strings in PHP. But it should solve the immediate issue with the current Regex approach. Supporting more complex syntax will likely require a more complex approach.

@selcukcukur
Copy link
Contributor

https://regexr.com/5psod Too many regex syntax errors.

@jasonmccreary
Copy link
Contributor

jasonmccreary commented Apr 1, 2021

@selcukcukur PHP uses PCRE - https://regexr.com/5pspt

@selcukcukur
Copy link
Contributor

@jasonmccreary I skipped that part, sorry 😅

@shaffe-fr
Copy link
Contributor

Hi @driesvints, this PR caused an exception for the following case:

@php($label = new HtmlString(__( 'forms.terms', [ 'link' => 'I don\'t'])))
<x-input-checkbox name="terms" :label="$label" required/>

The new regex seems to have issues with escaping (escaped apostrophe \' in my example).

@jasonmccreary
Copy link
Contributor

@shaffe-fr, it's currently a rather naive check for quoted strings. I'll see if I can adjust it to account for basic escaping. But I do worry we're pushing the limits of what this regex can do in a single pass.

@taylorotwell
Copy link
Member

@shaffe-fr just do <?php your code ?> 😓

@shaffe-fr
Copy link
Contributor

I understand that I can do this differently, I just wanted to indicate a small regression. Sorry for the inconvenience.

@driesvints
Copy link
Member Author

@shaffe-fr thanks for notifying us. For now please use Taylor's suggestion.

@selcukcukur
Copy link
Contributor

The last PR pushes the boundaries very much, I think this should not be applied.

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

Successfully merging this pull request may close these issues.

Components: The ")" character, this may cause an error
5 participants