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

[5.5] Prevent raw blocks from being restore in the wrong order #20072

Closed
wants to merge 1 commit into from

Conversation

sileence
Copy link
Contributor

There is a possibility that multiple raw blocks (@verbatim and/or @php) are rendered incorrectly if part of that code is commented with Blade (as Blade comments mean the raw placeholder will be removed too leaving the raw placeholders / blocks out of sync).

My solution was to give each raw placeholder an unique identifier, another idea would be to compile the Blade comments first but that could have other implications as we want to ignore the Blade code inside @verbatim and @php and also support the raw PHP tags as well.

@sileence sileence force-pushed the prevent_raw_blocks_mix_up branch from 3734ae1 to 03f767e Compare July 14, 2017 17:59
@taylorotwell
Copy link
Member

So this still has outstanding issues? I worry we are going to overcomplicate all this and still have edge case weirdness.

@sileence
Copy link
Contributor Author

sileence commented Jul 14, 2017

Well to be honest I just discovered this issue and it is present in Laravel 5.4 as well.

The following scenario:

{{-- @verbatim Block #1 @endverbatim --}} @verbatim "Block #2" @endverbatim

Will render Block #1 instead of Block #2

It's an edge case indeed but this wasn't caused by the previous PR*.

*Although this is probably still my fault. But let's not focus on that.

@sileence sileence changed the title Prevent raw blocks from being restore in the wrong order [5.5] Prevent raw blocks from being restore in the wrong order Jul 14, 2017
@sileence
Copy link
Contributor Author

I introduced the verbatim tag in 5.2. From 5.3 on blade comments are compiled to an empty string (#14029) and that's what causes the issue, because if there are @verbatim tags inside Blade Comments, the verbatim placeholder will be deleted, and right now they have to restored in the right order. With this PR the order won't matter anymore.

Of course with #20065 this issue is affecting the @phptags as well. But this PR should solve both cases.

Anyway, I hope I'm not introducing any new issues, but if I am, I'll gladly work in a solution.

@taylorotwell
Copy link
Member

Blade comments are not intended to comment out huge chunks of code. They are just meant for small text comments that you want in your Blade but not in your HTML. I don't want to go down this rabbit hole of trying to make them work like real language comments.

@sileence
Copy link
Contributor Author

@taylorotwell I know and I don't use them that way either tbh. But sometimes developers use them to comment large portions of code.

I think this code is more exact than the current version, anyway. Because we cannot rely on the verbatim / raw placeholders to be keep in place like before.

But anyway, if someone else raises an issue re this at least there is a proposal for a solution. I'll move on.

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.

2 participants