rcrowe / TwigBridge

Give the power of Twig to Laravel
MIT License
894 stars 168 forks source link

Fix: Facade Caller is never safe if the result is an object #426

Open jjanusch opened 2 years ago

jjanusch commented 2 years ago

This PR addresses an issue we were experiencing with Facades. We're upgrading an old project from Laravel 5.5 to 9.x that utilizes a few facades to generate code, specifically Laravel Collective's Form Builder. If you run this:

{{ Form.open() }}

it will autoescape the tag. This had previously not escaped the HTML for years on older releases, producing forms as expected.

I did a few hours of troubleshooting and determined the cause was this line: https://github.com/rcrowe/TwigBridge/blob/f4968efb99537cc1b37c5bf20280614aadc31825/src/Extension/Loader/Facade/Caller.php#L89

Form.open() returns an HtmlString object which has a __toString() method. The expectation is it would use that method to convert it to a String, but that doesn't happen because is_callable($result) should never return true on an object. Instead, it is intended to be run as:

is_callable([$result, '__toString']);

Unfortunately, switching this one line to just use that instead of the combined is_callable() && method_exists() resulted in some unexpected behavior. The commit that added this check said it was to fix a PHP 8 TypeError, and my change to usuing is_object() instead of is_callable() should satisfy the type errors, fix the bugs we're seeing, and maintain expected behavior.