framework
framework copied to clipboard
laravel
[9.x] Fixes blade tags issue #45424
solves #45424
The solution seems a bit hacky, but the idea, however, is simple:
- Without changing the regex pattern, we perform our first search to get matches.
- Then by using PHP tokenizer, we check if we are in trouble or not.
- If we are in trouble, we try to fix it by adding the next candidate section to the matched string to see if we can reach the proper closing parenthesis or not.
- We try the same process until we find the proper match for the closing parenthesis of the blade tag.
@include (
'partial1', ['hell)o' => ')-)--)']
)
in this example, regex finds the :
@include ( 'partial1', ['hell) first and the loop adds these portions until it reaches the closing:
)o' => '
)-
)--
'] ) <=== the closing one.
Performance:
-
Let me note that this part of the code is NOT run during each and every request/response life cycle since the compiled versions of blade files get cached.
-
Some tests are included to prove the fix.
As I know, Taylor likes to find a proper regex for it and does not like such solutions, maybe the regex can be found somewhere in open-source IDE syntax highlighters like VS code, or people from Jetbrain can help since they can highlight the syntax properly.
If we are going to attempt to solve that issue should we not add a new test ensuring that issue is actually fixed?
Yeah, I will add more tests, currently, I have changed the previous test which proves the bug to indicate the fix. But for now, it is a WIP. I should mark this as a draft. Slight tweaks are needed.
The first commit solves the problem for closing chars (without touching the original regex)
like: @include('a', [' :)))) '])
The second commit solves the problem for opening chars (but tweaks the original regex)
like: @include('a', [' :(((( '])
So if you want to stay really safe and avoid touching the regex, you can keep the first and discard the second one.
- Changing regex causes it to stop as soon as it reaches the first closing parenthesis, and do not match the opening and closing ones.
Can you provide some insight about what happens if it never finds a proper closing parenthesis?
Also, do you have some before / after benchmarks of running view:cache with this change on a code base with a fairly large amount of templates?
Since the user has to provide valid PHP syntax as input for the blade tags, it means that the closing parenthesis should be found or there is a syntax error in the first place, but yeah it is good to have a test about the result and the thrown error.
- Performance: I do not know how I should demo performance difference, but let's keep in mind: The new code is only executed if the regex fails to find a good match, so if you have an application with a lot of blade files that do not step over the bug, the tokenizer and all the added logic never get run even when you build the cache.
From experience, I know that the tokenizer is extremely fast. My laravel-microscope package runs the entire application code through tokenizer and all processes the tokens and this usually happens in a fraction of a second on a 10-years-old home PC.
Alright. Can you maybe add some sort of test case or behavior description for what happens if there is never a closing parenthesis found?
I added the test for the unclosed blade tag. It seems that they are considered a tag with no parenthesis.
I spent some time trying to break this today, but it seems to be holding up to everything I've thrown at it.
I also booted up a decently sized Blade only project from a few years ago that uses all the older style blade bits (i.e. not <x-whatever) and it seemed happy.
I ran php artisan view:cache on a loop for 100 iterations against the project with the compiler before and after this PR. The results:
Before: 11.34 seconds After: 11.98 seconds
Don't think there is anything performance-wise to worry about.
I think I may have found an edge case. The code below is broken on v9.47+, but works on v9.46.
<div>
<span @class([
'lt-text-xs lt-inline-block lt-py-1 lt-px-2 lt-rounded',
'lt-text-green-600 lt-bg-green-50' => !@empty($success),
'lt-text-yellow-600 lt-bg-yellow-50' => !@empty(
$warning
),
'lt-text-red-600 lt-bg-red-50' => !@empty($danger),
])>
{{ $value }}
</span>
</div>
This gets compiled to:
<span class="<?php echo \Illuminate\Support\Arr::toCssClasses([
'lt-text-xs lt-inline-block lt-py-1 lt-px-2 lt-rounded',
'lt-text-green-600 lt-bg-green-50' => !@empty($success),
'lt-text-yellow-600 lt-bg-yellow-50' => !<?php if(empty(
$warning
)): ?>,
'lt-text-red-600 lt-bg-red-50' => !<?php if(empty($danger)): ?>,
]) ?>">
And throws the error syntax error, unexpected token "<"
Context: https://github.com/lunarphp/lunar/pull/822
@rubenvanerk I think the fix is somewhat easy. Thanks for your report.