[10.x] Fix issue with @json incorrectly compiling when argument has one or more commas

[rojtjo]

I ran into the issue as described in #46908 myself recently and fixed it by parsing the arguments using preg_split instead of a simple explode.

The regex is a slightly modified version of the one from the SO answer below. It now also ensures that commas within parentheses and brackets are ignored.
https://stackoverflow.com/questions/18893390/splitting-on-comma-outside-quotes/18893443#18893443

We should probably add more cases to the jsonArgumentsProvider data provider before even considering merging this.

[driesvints]

Appreciate you sending in the PR. I just want to stress being careful with this because we've had numerous occasions in the past where we tried to adjust the compiler only to break other use cases for people.

[rojtjo]

I totally agree with you there, that's also the reason I marked it as draft.

Ideally we should handle all possible argument formats but I feel like that might be more trouble than it's worth. Especially since the workaround of just using {!! json_encode(...) !!} is almost just as easy.

[bert-w]

Your tests do not include the @json('a', JSON_PRETTY_PRINT) case (you can provide up to 3 arguments to @json) and there are more language structs than the ones that you define (like [...['a']] and a->?b->?c()).

Although your regex looks good and I cant find any obvious issues with some extra structs that i tested, it seems like the best way forward is to just use simple expressions whenever you are using @json (you probably want to delegate most of the logic to a controller/view-component anyway).

[rojtjo]

Your tests do not include the @json('a', JSON_PRETTY_PRINT) case (you can provide up to 3 arguments to @json) and there are more language structs than the ones that you define (like [...['a']] and a->?b->?c()).

I didn't include them since they are already tested in the test above, this test only makes sure the arguments are rendered correctly. But I totally agree with your second point, it needs a lot more cases. Also just realised that heredoc and nowdoc probably don't even work.

I guess to get this working for all possible cases we'd have to use nikic/php-parser but that definitely seems like overkill.

[rojtjo]

It indeed fails on heredoc and nowdoc strings as expected when parsing using regex.

To get it working using the php parser was actually a lot easier than expected. The php parser is currently only available when the dev dependencies are installed. This means we would need to add it as a dependency which is probably not something we would like to do for something like this

[bert-w]

Honestly it wouldnt be a bad idea to just bite the bullet and discard/deprecate this blade directive, in favor of something like:

{{ json($params) }}

// helpers.php
function json($value, $flags, $depth) {
    return new \Illuminate\Support\HtmlString(json_encode(...func_get_args()));
}

(HtmlString allows you to call it using {{ }} instead of {!! !!})

It makes no sense to apply all this difficult parsing of arguments.

The sole reason we can't do <?php echo json_encode($expression); ?> in the blade compiler is because we have defaults for the json arguments; this make parsing the string as php code a pain.

[valorin]

You can already do that with the Js helper class:

<script>
    var app = {{ Js::from($array) }};
</script>

I'd suggest adding more methods into that if there are use cases it doesn't support.

https://laravel.com/docs/10.x/blade#rendering-json

[bert-w]

Thx I've added a PR that exposes one of the functions in that class for this reason. The @json directive has already been removed from the docs, instead the docs only showcase Js::from() but that one explicitly echoes javascript code JSON.parse(...).

[rojtjo]

The only reason I'd use the @json directive over {!! json_encode(...) !!} is that it's just more aesthetically pleasing.

Also, I didn't realise the @json and @js directives were removed from the docs. Looking at the docs repo it seems that there were some attempts to add them back in, but they weren't accepted. I guess we might as well assume that the directives are deprecated then.