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

[10.x] Fix blade failing to compile when mixing inline/block @php directives #48420

Merged
merged 2 commits into from Sep 20, 2023

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Sep 15, 2023

Closes #48407

Updated the php regex from .*? to (?:.(?!(?<!@)@php))*?---long story short this is just a negative lookahead that matches everything that is not followed by @php while also allowing @@pphp.

Partial credit goes to @rikvdh

@calebdw calebdw force-pushed the blade_php branch 2 times, most recently from 58e23ae to 4b722c5 Compare September 15, 2023 16:14
@calebdw
Copy link
Contributor Author

calebdw commented Sep 15, 2023

Not sure why the test is failing, everything passed when I first created this PR and the only thing I changed was adding some more cases to the test...

@calebdw calebdw changed the title [10.x] Fix mixed inline/block php blade bug [10.x] Fix blade failing to compile when mixing inline/block @php directives Sep 16, 2023
@taylorotwell
Copy link
Member

Looks like they are failing on PHP 8.3. They are not failing on PHP 8.3 for other framework PRs that are open at the moment.

@taylorotwell
Copy link
Member

Could you mark as ready for review again once tests are passing?

@taylorotwell taylorotwell marked this pull request as draft September 18, 2023 18:31
@calebdw
Copy link
Contributor Author

calebdw commented Sep 18, 2023

@taylorotwell, tests are passing after rebase

@calebdw calebdw marked this pull request as ready for review September 18, 2023 18:52
@taylorotwell
Copy link
Member

The additional tests suggested by @crynobone do not pass:

    public function testCompilationOfMixedPhpStatements()
    {
        $string = '@php($set = true) @php ($hello = \'hi\') @php echo "Hello world" @endphp';
        $expected = '<?php ($set = true); ?> <?php ($hello = \'hi\'); ?> <?php echo "Hello world" ?>';

        $this->assertEquals($expected, $this->compiler->compileString($string));
    }

    public function testCompilationOfMixedUsageStatements()
    {
        $string = '@php (
            $classes = [
                \'admin-font-mono\', // Font weight
            ])
        @endphp';
        $expected = '<?php (
            $classes = [
                \'admin-font-mono\', // Font weight
            ])
        ?>';

        $this->assertEquals($expected, $this->compiler->compileString($string));
    }

    public function testMultilinePhpStatementsWithParenthesesCanBeCompiled()
    {
        $string = "@php ("
                ."\n    \$classes = ["
                ."\n        'admin-font-mono'"
                ."\n    ])"
                ."\n@endphp"
                ."\n"
                ."\n<span class=\"{{ implode(' ', \$classes) }}\">Laravel</span>";

        $expected = "<?php (\n"
                ."    \$classes = [\n"
                ."        'admin-font-mono'\n"
                ."    ])\n"
                ."?>\n"
                ."\n"
                ."<span class=\"<?php echo  implode(' ', \$classes); ?>\">Laravel</span>";

        $this->assertEquals($expected, $this->compiler->compileString($string));
    }

    public function testMixedOfPhpStatementsCanBeCompiled()
    {
        $string = "@php(\$total = 0)"
                ."\n{{-- ... --}}"
                ."\n<div>{{ \$total }}</div>"
                ."\n@php"
                ."\n    // ..."
                ."\n@endphp";

        $expected = "<?php (\$total = 0); ?>\n"
                ."\n"
                ."<div><?php echo e(\$total); ?></div>\n"
                ."<?php\n"
                ."    // ...\n"
                ."?>";

        $this->assertEquals($expected, $this->compiler->compileString($string));
    }

@taylorotwell
Copy link
Member

@calebdw please mark as ready for review again when tests are addressed

@taylorotwell taylorotwell marked this pull request as draft September 20, 2023 15:25
@calebdw
Copy link
Contributor Author

calebdw commented Sep 20, 2023

@taylorotwell,

There was a mistake in the third test (should be echo e(implode(...))), I've added the additional tests and corrected the mistake.

@calebdw calebdw marked this pull request as ready for review September 20, 2023 15:42
@taylorotwell taylorotwell merged commit 22d4530 into laravel:10.x Sep 20, 2023
20 checks passed
@taylorotwell
Copy link
Member

Thanks - will give this a shot.

@jorisleermakers
Copy link

I have faced an issue today, after updating Laravel from 10.24.0 to 10.25.0 by this change.
If you have a single-line PHP Statement that includes a ? or : the blade is failing to parse after this change.

Works:
@php ($var = ('a' == 'b'))

Does not work anymore:
@php ($var = ('a' == 'b' ? 'x' : 'y'))

It works again if you end the line with @endphp But that should not be mandatory.
@php ($var = ('a' == 'b' ? 'x' : 'y')) @endphp works

@QuentinGab
Copy link

Hi,
I can confirm that inlined @php is broken when using ternary.

@dwightwatson
Copy link
Contributor

Chiming in here as I've had trouble upgrading to the latest version and I suspect this may be the culprit. Curiously my tests pass locally but they fail in CI (GitHub Actions) and I can't narrow down why.

TypeError: Livewire\Volt\VoltServiceProvider::Livewire\Volt\{closure}(): Argument #1 ($value) must be of type string, null given, called in /home/runner/work/roomies/roomies/vendor/laravel/framework/src/Illuminate/View/Compilers/BladeCompiler.php on line 262 and defined in /home/runner/work/roomies/roomies/vendor/livewire/volt/src/VoltServiceProvider.php:42

I ended up removing Volt and then my tests passed in CI again.

I deployed to production and then some customers complained about receiving blank pages and I was able to replicate. Rolled back to 10.24.0 and the problem has gone away.

@mikescola
Copy link

This has caused massive issues, and unfortunately I didn't check my app's checkout page when I did the upgrade. Prime example of how tests give you a false sense of security. Can confirm this breaks @php($needsToIncrement++) as well, not just ternary.

@jorisleermakers
Copy link

@taylorotwell Maybe revert this commit in a next minor release? As it causes more issues then solving.

@calebdw
Copy link
Contributor Author

calebdw commented Sep 28, 2023

@jorisleermakers, @QuentinGab, @dwightwatson @mikescola, @driesvints.

Additional information is needed along with a minimum reproducible example---I added tests for the specific use cases above and they pass regardless of the old or new regex.

I'm specifically interested in if there are any @php tags before or after the ones that have issues.

taylorotwell pushed a commit that referenced this pull request Sep 28, 2023
@driesvints
Copy link
Member

We're reverting this and removing @php() from the docs. We won't recommend using it any further only @php @endphp.

@QuentinGab
Copy link

@jorisleermakers, @QuentinGab, @dwightwatson @mikescola, @driesvints.

Additional information is needed along with a minimum reproducible example---I added tests for the specific use cases above and they pass regardless of the old or new regex.

I'm specifically interested in if there are any @php tags before or after the ones that have issues.

I understand that, but I did not succeeded to reproduce the problem with only a simple blade view and inline php.
I assume that the error is not simply caused by the usage of ternary or nullish operator but in combination with something else.

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.

Blade fails to compile when a block @php directive follows an inline directive
7 participants