laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.52k stars 11.02k forks source link

Blade ternary `or` parser breaks when parsing regular ternary expression #23528

Closed DfKimera closed 6 years ago

DfKimera commented 6 years ago

Description:

When outputting the result of a ternary operation using Blade, and the string contains the or keyword, the parser incorrectly identifies it as being a Blade-style ternary operator ({{$variable or 'not defined'}}).

What happens is the contents of the or in the string gets replaced with the ternary macro (isset($variable) ? $variable : 'not defined'), breaking the output code.

Here's an example of the affected code:

image

Here's the Whoops screenshot showing the incorrect replacement.

image

If I surround the $variable with parenthesis, the parser does not kick in. (eg: {{ ($variable) ? 'lorem or ipsum' : 'dolor' }})

Steps To Reproduce:

On a clean Laravel project, add the following string to welcome.blade.php:

<p>{{ $variable ? 'lorem or ipsum' : 'dolor' }}</p>

Fixing the issue:

The RegExp that is failing is here:

https://github.com/laravel/framework/blob/90fc0babe1c854249c1750be9dfc6ab8f0937935/src/Illuminate/View/Compilers/Concerns/CompilesEchos.php#L101-L104

I believe (.+?) is incorrectly matching ?, which would signal the end of the variable/property name and indicate a ternary expression. There's probably a few other edge cases that break (since the feature is meant to also match things like $obj->property and $arr['key'], so it's very permissive).

I'm not very good with RegExp, so I haven't thought of a fix yet, but I'm willing to dig deeper if no one has a quick fix in mind.

browner12 commented 6 years ago

I thought we removed (or deprecated) the Blade 'or'?

DfKimera commented 6 years ago

@browner12 Apparently not, compileEchoDefaults() is still called for every echo type.

Nathanw commented 6 years ago

This regex might work instead:

public function compileEchoDefaults($value) 
{ 
    return preg_replace('/^(?=\$)(^[^\'"]+)(?:\s+or\s+)(.+?)$/si', 'isset($1) ? $1 : $2', $value); 
} 
browner12 commented 6 years ago

some references:

https://github.com/laravel/framework/commit/d8ea057642e8ff5cd198db8019fba472be01a9b6 https://github.com/laravel/framework/pull/22810

devcircus commented 6 years ago

assign the string to a variable. This has surfaced several times and @themsaid has mentioned it was probably a 'no-fix'. see #17934

browner12 commented 6 years ago

Let's deprecate the 'or' functionality then in 5.6, and remove in 5.7.

DfKimera commented 6 years ago

@Nathanw It misses some edge cases, like $arr["lorem ipsum?"] or "dolor".

I've just pushed a PR that accounts for those, feel free to point out any mistakes or things I may have missed.

DfKimera commented 6 years ago

@devcircus @browner12 I think it's worth a fix, it's a very handy feature and it breaks on code that's much simpler than #17934. One less thing to put in the migration guide (and replacing a couple hundred instances of these in existing views would be hell!)

browner12 commented 6 years ago

go ahead and fix it in 5.6, but I'm going to put a PR into 5.7 to remove it. It's a regex nightmare, and it's equally solved by the null coalesce operator (??) in PHP 7, that we now require.

the find and replace in your views is not that bad. I did it preemptively on a pretty large project a couple versions back, and it only took me about 10 minutes at most.

GrahamCampbell commented 6 years ago

It's simply not possible to use regex to parse this because it's not a regular language. Most of blade ultimately falls fowl to this, though we can do well in practice by limiting ourselves to finite nesting. One can show that we cannot recognise the infinite langauge of only correctly nested turnaries using the pumping lemma. This is because this language is context free.

DfKimera commented 6 years ago

@browner12 Agreed, with ?? the or keyword becomes redundant. I pushed #23530 for 5.6, can you take a look?

browner12 commented 6 years ago

https://github.com/laravel/framework/pull/23532