[5.8] Path of the view in compiled template causes regression with declare(strict_types=1) in templates

[mfn]

• Laravel Version: 5.8.2
• PHP Version: 7.2
• Database Driver & Version: n/a

Description:

[5.8] Provide a path of the view in compiled view added a new feature so that the first line of a compiled template contains the path to the view.

Unfortunately this breaks templates using <?php declare(strict_types = 1); in the first line.

PHP requires strict_types to be the very first statement in the PHP file. The error:

In b4b770a497bc79e9d39868f8567850fb50c4f817.php line 2:
                                                                           
  strict_types declaration must be the very first statement in the script  

Steps To Reproduce:

Template:

<?php declare(strict_types = 1);
// anything you want

Generated template in 5.7:

<?php declare(strict_types = 1);
// anything you want

Generated template in 5.8:

<?php /* /vagrant/project/resources/views/emails/template.blade.php */ ?>
<?php declare(strict_types = 1);
// anything you want

Were I work there's a strict policy about this requirement with no exceptions, same for type declarations. I've been using them since Laravel 5.1 and never had issues so far.

[bzixilu]

Unfortunately strict types declaration was not taken into account by myself, my fault.
Some info about usages: https://github.com/search?q=declare%28strict_types+language%3Ablade&type=Code

@taylorotwell do you see any possible pitfalls in the first scenario when we add template path as a last line in the compiled view as a fix for the issue?

[driesvints]

This might be a very subjective opinion but is it even our responsibility to fix this? Just don't put the declaration on top? I've indeed seen some examples which place it next to the <?php declaration but other major projects (Doctrine amongst else) do this two lines below. Even the upcoming PSR-12 Extended Coding Style requires it to be two lines below: https://github.com/php-fig/fig-standards/blob/master/proposed/extended-coding-style-guide.md#3-declare-statements-namespace-and-import-statements

[driesvints]

What I'm trying to say is that it's not very common to put things right after the <?php declaration and even should be avoided imo.

[taylorotwell]

I see no problem adding it to the end of the script instead.

[driesvints]

I have to retract my statement. Didn't notice it's a separate php statement. This indeed won't work: https://3v4l.org/6UfqU

[driesvints]

If we can generate it as follows it'll work.

<?php /* /vagrant/project/resources/views/emails/template.blade.php */ 
declare(strict_types = 1);

[driesvints]

@bzixilu would the phpstorm functionality still work with the above example?

[mfn]

So, how can we move forward here?

I know PhpStorm made it their news that they now support template debugging ( https://blog.jetbrains.com/phpstorm/2019/03/phpstorm-2019-1-beta/ ) and sure, it's useful.

Laravel itself though has no stake in it and if the original developer doens't come forward with a fix, shouldn't this be reverted then as it breaks functionality which is perfectly legal and fine and worked literally for years until this came along?

[bzixilu]

First of all, sorry for the long silence.

@driesvints if I understand you right, you suggest to insert a path to the first <?php ...> tag in the compiled view if it exists. The solution might work indeed, but it's a little bit harder than just adding the path at the end of the compiled view since it requires Blade compiler as well as PhpStorm to go through the file and find this first PHP tag.

Besides, there is a couple of other minor things. It's easier for an eye to find a debug info in a permanent place like at the end of the file. Furthermore, in the future, we will also need to provide line mappings in the debug info. If we insert those lines somewhere in the middle of the compiled view, it will look strange.

So I would prefer to just move the comment with a view path to the end of the file (please take a look at pull-request #27914)
However, all mentioned above concerns are not very essential and may be subjective and if you insist I will implement the fix in the suggested way.

[driesvints]

@bzixilu this looks good!

[driesvints]

PR was merged.

[mfn]

Thank you @bzixilu 👍

[ArlonAntonius]

Hi everyone,

We noticed an error in our package caused by this PR and I just wanted to check if that behavior is intended.

There's a simple test that checks whether our package can override the basic views. All we do is simply create a view through the filesystem, then check if the rendered view has the text we provided in it.

Normally tests pass with ease, but since this PR got merged, we get this error.

1) Hyn\Tenancy\Tests\Filesystem\LoadsViewsTest::loads_additional_views
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'bar'
+'bar\n
+'

Just wanted to check with you if this behavior was intentional or unintentional.

[mfn]

It's a side effect of the change: https://github.com/laravel/framework/pull/27914/files#diff-314543ef8cd684a5a1ca0d2388d419ebR122

[ArlonAntonius]

@mfn Yeah, appreciate the reply, so this is intentional and will stay like this? :)

[driesvints]

I think it's best that we remove the newline if there's no path present.

[bzixilu]

Well, for me it's not exactly clear how this behavior could be caused by the change mentioned above since I would expect in failed test something like:

--- Expected
+++ Actual
@@ @@
-'bar'
+'bar\n
+<?php /*  */ ?>
+'

if no path specified in the view. But probably I missed something.

But I totally agree that we should not add the path if it's not specified in the view.
Please find the pull-request with the fix: #27976
I hope it helps to resolve the issue above.

[driesvints]

PR was merged.