[5.5] Allow Blade to render multiple verbatim and php blocks

[sileence]

Simple solution for this problem: #21897 (comment)

To allow Blade to render raw blocks (@verbatim and @php) in the intended place.

[sisve]

This would change the behavior for every existing 5.5 installation that uses both these tags. Isn't this something that should target 5.6?

[sileence]

@sisve could you please proof (with a unit test if possible) how the behaviour would be change? Other than fixing the bug, of course.

[sisve]

The bugfix is the changed behavior; blocks are suddenly executed/parsed in another order. This new order may be the "correct" one, but it doesn't change the fact that existing sites will have their views rendered differently.

[taylorotwell]

Is this fixing the literal case of stringing all that stuff on one line like that? Is that the only case we are fixing here?

[sileence]

@taylorotwell no, it can be multilines as well, I added another test to prove it.

[taylorotwell]

@themsaid do you know of any problems with this?

[themsaid]

@taylorotwell honestly I believe we'll never get this 100% right :) I don't have a specific problem against this PR but in general most times when we touch this area someone reports a breaking change.

[sileence]

In case it helps, I refactored the code a bit, to show the intention of adding a unique placeholder for each raw block vs the current version which adds the same placeholder for all of them relying they were going to be restored in order (which is no longer the case).

[sileence]

To show the process:

{{ $first }}

@php
    echo $second;
@endphp

@if ($conditional)
    {{ $third }}
@endif

@verbatim
    {{ $fourth }} 
@endverbatim 

@php echo $fifth; @endphp

Will be temporary transformed to:

{{ $first }}

@__raw_block_1__@

@if ($conditional)
    {{ $third }}
@endif

@__raw_block_0__@

@__raw_block_2__@

Then Blade will compile:

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

@__raw_block_1__@

<?php if($conditional): ?>
    <?php echo e($third); ?>

<?php endif; ?>

@__raw_block_0__@

@__raw_block_2__@

Finally the raw blocks will be restored, but as each block has a unique ID placeholder they should be restored in the intended order:

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

<?php
    echo $second;
?>
<?php if($conditional): ?>
    <?php echo e($third); ?>

<?php endif; ?>

    {{ $fourth }} 
 
<?php echo $fifth; ?>

[dafonso]

@sisve and @taylorotwell: I don't get it how this should target only a major version. This is a bug. Or is it an expected behaviour that I put @php and @verbatim directives on a template and they will be rendered incorrectly? Or is one supposed to construct a view based on what Blade will (wrongly) compile it to? I think the expected behaviour is that I create a view and directives are compiled into the order they are on the view, isn't it? #21897

[taylorotwell]

How does this work if someone @include another view in the middle of all this stuff?

[sileence]

@taylorotwell the same as @include is roughly a call to View::make and not a merge. I modified the test to prove it.

[sisve]

@dafonso It may be a bug, but it's not blowing things up. People run into it, chants something evil, and then codes a workaround of some kind. This is something the developer will detect when developing.

Changing this now, at 5.5.20 (I presume), is a breaking change that will affect people. I'm sure that I do not have enough automatic tests that would detect these type of changes in the generated blade views. Do people expect a full manual testing round for the entire application in every 5.5.x release?

What if they didn't even mean to upgrade laravel/framework, but install the sqlite support according to the migration docs, or the ses support according to the mail docs, that tells them to execute composer update which will upgrade all packages?

That's why I think that this issue should target 5.6; there's a higher acceptance for breaking changes and those are documented in upgrade docs that tells people what changed and how to work around it. Those types of instructions are not present in a 5.5.x release, just an entry in a changelog.

[sileence]

@sisve people will expect bugs to be solved in minor releases. It's even in the description of Laravel:

Bug fixes for 2 years, security fixes for 3 years.

You shouldn't develop apps relying on behaviour provided by bugs. A workaround is something different. A woraround means not using the feature with problems, not relying on the bug, for example: using <?php ?> instead of @php @endphp and that has nothing to do with this fix.

[dafonso]

@dafonso It may be a bug, but it's not blowing things up.

@sisve: Yes, it is blowing things up... Why? I upgraded from 5.4 to 5.5.19 a few days ago and one of my views that had @php and @verbatim directives blew up... First I was completely lost as to why, then started debugging... and found the bug that I reported on #21897. Took me some hours to understand it... So I absolutely agree with @sileence last comment.

With this in mind, take time and reproduce the bug with the steps on #21897 and if you still find this is a behaviour change only for a major release and not a major breaking bug, then my understanding of bugs, major/minor releases and software development in general may be completely lost at sea. And I'm completely open to other views on this issue that make me change my opinion, but atm I'm confused with the comments of not wanting to address this asap.

[sisve]

There is nothing in the docs that states that the current order is wrong. It's reasonable to believe that people will just shrug this off as a weird thing and keep coding. I base this on earlier discussion on the order of rendering/includes of views, sub-views and templates. Or perhaps I base this on the idea that blade comments cannot comment out everything, like @php directives. Or perhaps that blade comments aren't supposed to comment any larger text block at all. My point is that there are many odd things in Blade that you accept and just ignore and move on.

The current behavior was known at the release of Laravel 5.5.

[...] we cannot rely on the verbatim / raw placeholders to be keep in place like before.

@dafonso Your example involves upgrading a major Laravel version, and things do break in those. As I've mentioned earlier, I believe people spend more time testing their applications in these upgrades than in a 5.5.x+1 upgrade.

Remember, this isn't a week or two after the release. Two months have now passed and we've had 18-19 minor releases since the release of 5.5. Do people expect it to change now? Can't it wait until 5.6?

I'm not saying that this isn't a bug, or that this should never be fixed. I'm pointing out the lack of analysis on how this affects existing Laravel 5.5 installations. That's what I do, I am bitter 24/7 and think of ways a PR may affect things that havn't been mentioned in the issue.

This is a breaking change; it can potentially cause other things to fail. That does not mean that it can not be targetting 5.5. That's up to a project manager to evaluate. Breaking changes aren't forbidden in minor releases, we just need to be clear of potential problems and make sure that those are communicated in proper forms (which in this case is at least the changelog).

[devcircus]

This is why I cringe every time someone comes up with a shiny new blade directive they want to include in the core. Blade has well known limitations and those become more obvious with each new directive. Usually a "fix" breaks something else. If I have code in a blade file that's a little complicated or hard to make work well with blade, I just ditch the directives and move on.

Just giving @sisve a thumbs up.

[sileence]

@sisve alright, it's easy to get lost in a discussion like this one and this could happen in virtually every PR to L5.5. I'd prefer if you or anyone else help having a look at the changes and try to determine ways this could actually break functionality with actual code examples and tests?

I added several tests and even documented the process. I could still be making a mistake, that's true but in any case it helps if other people read the code, analyse the problem itself and the solution itself.

[taylorotwell]

Can we show a real scenario of how this fix would break an existing Laravel 5.5 application?