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 fails to compile when a block @php directive follows an inline directive #48407

Closed
calebdw opened this issue Sep 14, 2023 · 6 comments · Fixed by #48420
Closed

Blade fails to compile when a block @php directive follows an inline directive #48407

calebdw opened this issue Sep 14, 2023 · 6 comments · Fixed by #48420

Comments

@calebdw
Copy link
Contributor

calebdw commented Sep 14, 2023

Laravel Version

10.22.0

PHP Version

8.2.10

Database Driver & Version

No response

Description

Hello!

Blade currently fails when a block @php directive follows an inline @php(...) directive, this is because the regex skips over the block start and matches on the block end.

Below is a minimum reproducible example:

<html>
    <body>
        @php($total = 0)
        {{-- ... --}}
        <div>{{ $total }}</div>
        {{-- ... --}}
        @php
            // ...
        @endphp
    </body>
</html>

I understand there have been several issues created on this topic (some are several years old). However, this is still an issue and there is nothing in the docs stating that the two syntaxes are incompatible. This bug either needs to be fixed or the docs need to be updated to state their incompatibility.

image

Thanks!

Steps To Reproduce

  1. Add an inline php statement to a blade file @php(...)
  2. Later in the file, add a block php directive:
    @php
        // ...
    @endphp
    
  3. Fail
@tylernathanreed
Copy link
Contributor

This was called out in #45330 by @rikvdh, and was closed with the directive of "Don't mix these up", and favoritism to just use raw <?php tags.

If this issue gets closed for the same reasons, then I suggest we submit a PR to laravel/docs to at least highlight the problem, so that developers don't waste hours of research & debugging only to find that there's a bug in the framework that isn't going to get fixed any time soon.

@github-actions
Copy link

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

@crynobone
Copy link
Member

crynobone commented Sep 15, 2023

The anyone want to contribute, the new code need to ensure existing tests as well as following still return successful:

    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));
    }

@rikvdh
Copy link
Contributor

rikvdh commented Sep 15, 2023

To be fair.. Since my PR and issue I re-thought this and approach this a different way.

Your priorities should be the following:

  1. Don't use PHP in views!!
  2. If you REALLY want to, use regular <?php ... ?> tags.

Using PHP in views is not always avoidable but is almost never the way to go forward.
I think that there should be a debate maybe on removing the @php-tags as a whole in the next major.. imo there is no real reason to have them anyway.

@calebdw
Copy link
Contributor Author

calebdw commented Sep 15, 2023

@rikvdh,

As you said, sometimes it's not avoidable...

If you REALLY want to, use regular tags.

This argument is kind of ridiculous in my opinion---you're basically saying that a feature in the framework exists, but nobody should use it. I'm fine if the framework wants to remove them in 11, but until then it's a feature that should be supported.

Although another option is to just remove the inline directive and enforce that everyone uses the @endphp tag.

A lot of the headaches in the regex-based parser could be avoided however, by Laravel supporting an AST parser as a first class citizen which can also be used for highlighting, indentation, formatting, etc.

Even better would be first class support for a blade language server similar to the one for antlers (Statamic template engine)

@rikvdh
Copy link
Contributor

rikvdh commented Sep 15, 2023

@calebdw I 100% agree. I also want that features that exist should be supported and work.
But if you read up on the history I have with this issue:

My initial fix:

#45333

Which was released and shortly after reverted and then with the updated regex:

#45390

Which was working but not accepted. I now hang to my argument that the feature is better gone all together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants