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

Blade renderer breaks when mixing 2 @php-syntaxes in the same view #45330

Closed
rikvdh opened this issue Dec 15, 2022 · 18 comments · Fixed by #45333
Closed

Blade renderer breaks when mixing 2 @php-syntaxes in the same view #45330

rikvdh opened this issue Dec 15, 2022 · 18 comments · Fixed by #45333

Comments

@rikvdh
Copy link
Contributor

rikvdh commented Dec 15, 2022

  • Laravel Version: 9.43.0
  • PHP Version: 8.1.13
  • Database Driver & Version: n/a

Description:

When mixing @php(..) and @php/@endphp later in the view the renderer breaks and gives preference to the @php/endphp pair above the parentheses.

Same issue as just posted, but tested in V9. @driesvints your excuse of "don't use it" is from my perspective not really an excuse. If you have a view of 400+ lines add @php/@endphp somewhere in the bottom and you didn't notice that someone else added @php(..) somewhere higher up in that same file you pull your hair out on why it doesn't work. Because the errors get quite weird when mixing things with other directives in between.

Steps To Reproduce:

See https://github.com/rikvdh/laravel-broken-blade-view/blob/v9/resources/views/welcome.blade.php

You expect 'goodbye' as output. But this breaks.

The rendered view:

<?php($hello = 'welcome')
@php
    $hello = 'goodbye';
?>

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

<?php /**PATH /Users/rikvdh/tmp/broken-view/resources/views/welcome.blade.php ENDPATH**/ ?>%                  
@driesvints
Copy link
Member

Don't mix these up.

@driesvints
Copy link
Member

If you want, feel free to send in a PR that would provide a solution to this.

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 15, 2022

It is a bit lame to just 'shut your eyes' for this obvious bug. There is no mention in the documentation on not mixing these kinds of methods, that makes it a bug. Keeping the issue open for someone else to work on or when I get the time to do it if you don't feel like it.

@tylernathanreed
Copy link
Contributor

tylernathanreed commented Dec 15, 2022

@driesvints

I'm confused as to why this was closed. According to the documentation (https://laravel.com/docs/9.x/blade#raw-php), both @php(...) and @php ... @endPhp are supported, and nowhere does it state that you can't use both.

If not being able to mix the two is the official stance of Laravel, then it needs to be called out in the documentation. Otherwise, this is a bug, and should be treated as such.

@driesvints
Copy link
Member

We're not going to spend time looking into this ourselves. You're welcome to send in a PR. This is all easily solved by just using regular PHP tags.

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 15, 2022

@driesvints Why dont deprecate @php tags then entirely? If the behavior is buggy and wont be fixed and the answer is 'just using regular PHP tags'.

@ssddanbrown
Copy link

@rikvdh dries didn't say this won't be fixed, he welcomed a PR. Deprecating would like put an unnecessary change on many existing use-cases.

Just a note, this thread was shared on reddit by the OP so may receive additional activity due to this.

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 15, 2022

@ssddanbrown From my perspective if something is buggy and you are advised to use something else by maintainers the feature has no reason to exist.

The reason I've shared this on Reddit is not really part of this actual issue. But I'm quite new into the community and am used to a lot of other OS communities and worked a lot of places and program PHP for over 15 years and the way of communicating and closing issues with a 'dont use it' within 1 minute feels very rude to me and something I've never seen before. I understand it in a way, but on the other hand doesnt feel very welcoming to someone trying to contribute to this project.

'yeah you can submit a PR, but idc.' is the tone it arrives at for me.

Feel free to lock this issue for commenting.

@lewislarsen
Copy link

lewislarsen commented Dec 15, 2022

Don't mix these up.

Yikes. That's a bad look Dries.

@ericjamesturner
Copy link

ericjamesturner commented Dec 15, 2022

rikvdh: "There's ice on the sidewalk"
Dries: "Don't slip! or feel free to fix it..."
rikvdh: "I'd rather go complain on reddit until someone else fixes it"

I agree with dries. - this is something that isn't a big issue, is likely to affect very few people, and those people can just be more careful, or if those people feel strongly about it, fix the issue for the benefit of the community... or you can get a refund if you'd like

@rikvdh
Copy link
Contributor Author

rikvdh commented Dec 15, 2022

rikvdh: "I'd rather go complain on reddit until someone else fixes it"

That is a little bit short sighted. I can fix it myself, but I don't feel very invited to do so. I remove the link to this issue on Reddit to prevent making this personal to anyone. That is not the goal.

@ericjamesturner
Copy link

ericjamesturner commented Dec 15, 2022

rikvdh: "I'd rather go complain on reddit until someone else fixes it"

That is a little bit short sighted. I can fix it myself, but I don't feel very invited to do so. I remove the link to this issue on Reddit to prevent making this personal to anyone. That is not the goal.

Nobody should need to invite you to "do the right thing".

The people who are behind this package built it, maintain it, keep it secure, release new features, documentation, etc...

For free. And they give away all of the source code. For free.

Shouldn't you just fix the code, and submit a PR... to give back to the community you are taking so much from? For Free?

They do not need to invite people to make improvements, they are not the feelings police. They provide a package in an open source way - for free - and if you find something wrong with it, you can either fix it, or you can report the bug and see if they agree it is somewhere they should invest time/resources into fixing. If the package maintainers don't feel like it is a good use of time/resources, you are free to fix it yourself if you feel strongly about it.

There is a condition which basically makes it so when people find something, they feel a sense of "This is big because I found it!" No matter how small the issue may be, because you found it, it is more important to you than anyone else on the planet. The maintainers here see this same thing every say, and they have to triage and determine what issues to spend time on, and where not to - the core team is limited, they have deadlines to meet to keep the releases rolling, and don't have an unlimited amount of time to chase every tiny bug, build tests for it, ensure the fixes are backwards compatible, don't break existing code, etc etc..

Especially not for a bug that is rarely reported, if ever. A bug that has an insanely low impact, will always be caught during development, poses no security risk, and is only present when someone isn't paying attention to the code that is in the file.

@driesvints
Copy link
Member

driesvints commented Dec 15, 2022

Hi all. I'm sorry if you feel I was a bit short sighted in this. My two cents are just that this is an easy fix by using PHP tags. I don't see a big bug here and that's my personal opinion. We're open to PR's and welcome bug fixes for this but we won't be spending time on this ourselves hence me closing this. If you don't like the way we want to maintain our projects then I'm sorry but we just do what we feel that works for us.

@taylorotwell
Copy link
Member

Hey there - if anyone wants to improve this send a PR.

@taylorotwell
Copy link
Member

We close issues we don't plan to work on. Google still surfaces closed issues. Anyone can still search closed issues. That's how we work. 👍

@tylernathanreed
Copy link
Contributor

@taylorotwell, @driesvints

If the expectation is that the community has to fix small bugs like this, is there an easy way to find closed, but unresolved, issues like this?

I feel like if I wanted to start contributing, I'd start on issues like these.

taylorotwell pushed a commit that referenced this issue Dec 15, 2022
taylorotwell added a commit that referenced this issue Dec 21, 2022
…s when @endphp is also in the same file, + unit test, fixes #45330 (#45333)"

This reverts commit 2427cec.
taylorotwell added a commit that referenced this issue Dec 21, 2022
…s when @endphp is also in the same file, + unit test, fixes #45330 (#45333)" (#45389)

This reverts commit 2427cec.
@taylorotwell
Copy link
Member

@tylernathanreed I'm not sure. We could likely tag them.

@tylernathanreed
Copy link
Contributor

@taylorotwell, that sounds fantastic! I'd be happy if something like was done for issues closed in this manner moving forward.

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 a pull request may close this issue.

7 participants