[8.x] Fix stringable implementation

[ibrasho]

The current implementation compiles echo statements with ternary operators (e.g. {{ $test ? "1" : "2" }} to the following code which throws a FatalError on PHP 8:

<?php echo e(is_object($test? "1" : "2") && isset(app('blade.compiler')->echoHandlers[get_class($test? "1" : "2")]) ? call_user_func_array(app('blade.compiler')->echoHandlers[get_class($test? "1" : "2")], [$test? "1" : "2"]) : $test? "1" : "2"); ?>

This PR tries to fix that issue by generating a compiled view without an included ternary operator.

[taylorotwell]

@lukeraymonddowning looks like string handling is broken if the person is already using ternarys.

[ibrasho]

This caused an issue since I'm using Livewire and @entangle blade directive is already using a ternary operator.

[ibrasho]

@lukeraymonddowning Any reason we are outputting ternaries in the complied view? I switched to just calling a method in the Blade compiler (since we are already resolving the compiler) instead.

[taylorotwell]

@ibrasho why make the call to the echo handler method in every compiled echo instead of only when there are actually echo handlers that are registered?

[lukeraymonddowning]

@ibrasho ternary isn't necessary, although I think you've removed the ability for it to skip over if no handlers have been defined, which causes lots of other tests to fail

[ibrasho]

@ibrasho why make the call to the echo handler method in every compiled echo instead of only when there are actually echo handlers that are registered?

The main reason I did that was that having different compiled outputs for echo tags was a bit confusing and made debugging hard for me. Having more consistent compiled output (regardless if the user has defined stringable handlers or not) seemed like a better idea.

WDYT @taylorotwell @lukeraymonddowning? I can either fix the tests, or restore the empty handler check?

[lukeraymonddowning]

@ibrasho I would say 2 reasons for not having logic if no handlers have been defined:

1. Loads of test cases need updating if you don't (that's why the test suite is currently failing).
2. For those who don't need the functionality, they'll get a minor increase in performance. Nothing too noticeable, but worth factoring in.

[ibrasho]

I'd personally prioritize maintainability over an unnoticeable performance improvement. But I'll leave that decision to @taylorotwell .

I updated the commit to add the empty handlers check back.

[lukeraymonddowning]

Thanks for the fix @ibrasho 👍 out of curiosity, how often do you find yourself debugging and reading through the compiled PHP?

[ibrasho]

Thanks for the fix @ibrasho 👍 out of curiosity, how often do you find yourself debugging and reading through the compiled PHP?

Not frequently enough to make it matter honestly. But I'd rather I don't waste any time doing that in the first place. 😁

[ibrasho]

@taylorotwell it looks like that change also breaks semicolons in echo statements. Is that even a supported feature that we should bring back? (Check livewire/livewire#2935)

[lukeraymonddowning]

Could we strip out the last semicolon in the value? Can you see an issue with that?

[lukeraymonddowning]

Just done a test and it does work. Here's a test:

public function testHandlerLogicWorksCorrectlyWithSemicolon()
    {
        $this->expectExceptionMessage('The fluent object has been successfully handled!');

        $this->compiler->stringable(Fluent::class, function ($object) {
            throw new Exception('The fluent object has been successfully handled!');
        });

        app()->singleton('blade.compiler', function () {
            return $this->compiler;
        });

        $exampleObject = new Fluent();

        eval(
        Str::of($this->compiler->compileString('{{$exampleObject;}}'))
            ->after('<?php')
            ->beforeLast('?>')
        );
    }

And here's the updated method logic:

protected function wrapInEchoHandler($value)
    {
        return empty($this->echoHandlers) 
            ? $value 
            : 'app(\'blade.compiler\')->applyEchoHandler(' . Str::beforeLast($value, ';') . ')';
    }

Might be a good case for a test dataprovider, and throw it a bunch of common use cases

[taylorotwell]

I'm not sure I totally follow the semicolon issue. What is the problem?

[lukeraymonddowning]

@taylorotwell basically if the value inside the {{ }} ends in a semicolon, it throws an error because it gets wrapped in a method call, and you can't have a semicolon in a method parameter.

[ibrasho]

For an example, this:

{{ $a; }}

now compiles into:

<?php echo e($a;); ?>

or if a stringable is defined:

<?php echo e(is_object($a;) && isset(app('blade.compiler')->echoHandlers[get_class($a;)]) ? call_user_func_array(app('blade.compiler')->echoHandlers[get_class($a;)], [$a;]) : $a;); ?>

Both cause a ParseError (notice the misplaced semicolons).

[ibrasho]

Could we strip out the last semicolon in the value? Can you see an issue with that?

I don't see any issues with doing that, but I want to make sure @taylorotwell agrees this is the way forward.

[taylorotwell]

Personally this whole feature is starting to give me a headache 😬

Is there any faster way to do this than calling into the container to resolve something on every single echo in your application's templates? That could be thousands of times per template when showing a lot of data. Calling into the container has to check for aliases, raise a before resolving event, etc.

If we can't think of a clean, fast way to solve this I may just remove the feature.

[lukeraymonddowning]

Let me take another look this evening. Might be able to resolve it once at the top of the template.

I think the original issue is solved to be honest. I'm struggling to see how the semicolon issue wasn't a problem previously, since it's also broke on echos it doesn't touch.

[lukeraymonddowning]

@taylorotwell couldn't add to this PR so opened a new one: #37613

[ibrasho]

@taylorotwell I'd suggest adding a new class and injecting it into the Illuminate\View\Factory like $__env and $app ? That way we don't need to resolve in every echo statement.

I just updated the Illuminate\View\Factory with the ability to echo objects since it's already injected in views (and honestly this functionality is not limited to the Blade compiler).

[lukeraymonddowning]

@ibrasho doesn't this change how you'd declare a handler seen as the compiler no longer has the stringable method?

[ibrasho]

I made a simple change to add that method back to BladeCompiler.

[taylorotwell]

Fixed. Thanks.