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

fix for single line @php statements but also for multi-line @php with parentheses and @endphp #45390

Closed
wants to merge 2 commits into from
Closed

fix for single line @php statements but also for multi-line @php with parentheses and @endphp #45390

wants to merge 2 commits into from

Conversation

rikvdh
Copy link
Contributor

@rikvdh rikvdh commented Dec 21, 2022

I've changed the regex to work a bit different. At first we had a negative lookahead on the parentheses. But as seen in #45388, people actually use parentheses which makes it look like a single-line statement instead, but then include the @endphp. Since the regex was greedy and ate other @php-statements in the process of searching for full blocks it didn't work.

I've added another test and change the regex to be omitting possible other @php-blocks making only the @php/@endphp pairs really working.

@taylorotwell
Copy link
Member

Tests failing.

@taylorotwell taylorotwell marked this pull request as draft December 22, 2022 15:12
@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 22, 2022

Sorry I noticed. I need to continue breaking my head some more on that regex..

@rikvdh rikvdh marked this pull request as ready for review December 23, 2022 06:53
@donnysim
Copy link
Contributor

donnysim commented Dec 23, 2022

To be fair I think #45388 uses the @php incorrectly, if you put brackets you don't put @endphp

@php (
    $classes = [
        'admin-font-mono', // Font weight
    ])
@endphp

should be

@php
    $classes = [
        'admin-font-mono', // Font weight
    ]
@endphp

or just

@php (
    $classes = [
        'admin-font-mono', // Font weight
    ])

with his version even the IDE complains that something is wrong and it's missing an opening statement because of how it perceives the usages, I'm surprised that it doesn't compile to <?php ... ?> ?>

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 23, 2022

@donnysim I agree, but it breaks past behavior and this makes all cases work. If you put @endphp or not.

@donnysim
Copy link
Contributor

But is it worth fixing framework for someones bad code? Maybe this could be moved to 10.x as a breaking change?

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 23, 2022

@donnysim I leave that up to maintainers to decide. I would like a fix in 9.x as well since I've now faced my earlier issue a couple of times. The past behaviour of @php(..) was a bit buggy depending on its surroundings.

@donnysim
Copy link
Contributor

donnysim commented Dec 23, 2022

@rikvdh I'm kind of confused here, I just tried

@php(
    $hello = 'Hello World'
)
@endphp

{{ $hello }}

and got

Illuminate\View\ViewException : Undefined variable $hello (View: F:\Laragon\boilerplate\resources\views\render.blade.php)

how is his code working then? 🤔 if I use that variant without echo it prints me the php code instead of a blank string as it should

EDIT: oh I see, iif you put space after @php ( then it works, if you don't, it doesn't

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 23, 2022

@rikvdh I'm kind of confused here, I just tried

@php(
    $hello = 'Hello World'
)
@endphp

{{ $hello }}

and got

Illuminate\View\ViewException : Undefined variable $hello (View: F:\Laragon\boilerplate\resources\views\render.blade.php)

how is his code working then? 🤔 if I use that variant without echo it prints me the php code instead of a blank string as it should

Woah.. I was going to say: 'I dont know'. But if you put a space before the opening parenthesis ot works. ;-)

Apparently there is a difference between

<?php (
    $hello = 'Hello World'
)
?>

<?php echo e($hello); ?>

and

<?php(
    $hello = 'Hello World'
)
?>

<?php echo e($hello); ?>

probably some weird PHP scoping thing. I've not bothered to investigate any further, but with the parenthesis adjacent it seems that the variables are scoped. This is also current behavior without my fix.

@donnysim
Copy link
Contributor

Yeah, the space makes it a full php opening and closing match 😅 so the whole statement got confusing. IDE also doesn't recognize it as a separate case which got me more confused that it should. Kind of seems like a gnarly case to crack using regex.

@taylorotwell taylorotwell marked this pull request as draft December 23, 2022 14:26
@rodrigopedra
Copy link
Contributor

If you always keep a space after the PHP opening tag, it should work.

return $this->storeRawBlock("<?php {$matches[1]} ?>");

added a space before the closing tag for symmetry only

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 28, 2022

@taylorotwell Why did you mark it as a draft? imo this is ready.

@rikvdh rikvdh marked this pull request as ready for review December 28, 2022 06:54
@taylorotwell
Copy link
Member

Honestly I don't have any appetite to change this and make the regex more complicated and potentially have another breaking change. The issue isn't that important to me nor is it a pressing issue in the Laravel community.

@rikvdh
Copy link
Contributor Author

rikvdh commented Jan 3, 2023

@taylorotwell I don't see how this is breaking in its current form. imo the current behavior is buggy and needs fixing.

@caendesilva
Copy link
Contributor

Just chiming in here that I also think this feels like a bug. It's rather unexpected to me that you can't use both single-line and multi-line directives. Maybe a note should be added to the docs. I'd be happy to do it if it's desired.

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.

5 participants