yiisoft / yii2

Yii 2: The Fast, Secure and Professional PHP Framework
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
14.24k stars 6.91k forks source link

Widget::end will crash if widget is configured by closure in di container #20267

Open jrajamaki opened 1 week ago

jrajamaki commented 1 week ago

What steps will reproduce the problem?

Using basic app as an example, create your own widget, e.g.

class ActiveForm extends yii\bootstrap5\ActiveForm {}

and configure di container to replace the original implementation using closure:

'container' => [
    'definitions' => [
        yii\bootstrap5\ActiveForm::class => fn ($container, $params, $config) => new ActiveForm($config),
    ]
],

Now when accessing basic app's "Contact" page, then app crashes with error "Cannot use object of type Closure as array".

What is the expected result?

It would work.

What do you get instead?

"Cannot use object of type Closure as array" error.

Additional info

Similar to yiisoft/yii2-debug#234. Caused by commit 0dbc4d3f35b1bfa0c4217ae744e30a139c8ad2c8.

Q A
Yii version 2.0.51
PHP version 8.3.9
Operating system macOS 14.5 Sonoma
jrajamaki commented 6 days ago

Would you agree to take PR for this?

If so, how it should work?

samdark commented 6 days ago

Yes. Sure. PR is very welcome. Skipping the check sounds alright.

rob006 commented 6 days ago

We could save class in begin() and use it in end() instead of getting definition from container.

jrajamaki commented 5 days ago

We could save class in begin() and use it in end() instead of getting definition from container.

That's how it works already. end() just has some safeguards so that developer wouldn't do anything stupid like trigger end() without begin() or mix two widgets:

WidgetA::begin([...]);
WidgetB::begin([...]);
WidgetA::end();
WidgetB::end();

The initial problem is that WidgetA might not be WidgetA because begin() uses Yii::createObject() to create the widget and widget's class might change if configured so in container configuration. But get_called_class always returns WidgetA in end() and wouldn't match the widget's class created in begin() if reconfigured. https://github.com/yiisoft/yii2/commit/0dbc4d3f35b1bfa0c4217ae744e30a139c8ad2c8 tried to fix this but didn't consider scenario where container configuration is callable.

rob006 commented 5 days ago

I mean we could this in begin():

self::$resolvedClasses[get_called_class()] = get_class($widget);

and this in end():

$calledClass = self::$resolvedClasses[get_called_class()] ?? get_called_class();

then we could remove these lines completely:

https://github.com/yiisoft/yii2/blob/3c75ff1043cdfc3c0c78ad8a4b477b5894223a5a/framework/base/Widget.php#L108-L110

jrajamaki commented 4 days ago

I mean we could this

Seems good, this is something what I had in my mind

public static function end()
{
    if (empty(self::$stack)) {
        throw new InvalidCallException('Unexpected ' . get_called_class() . '::end() call. A matching begin() is not found.');
    }

    $widget = array_pop(self::$stack);
    $calledClass = get_called_class();

    if (Yii::$container->has($calledClass)) {
        $definition = Yii::$container->getDefinitions()[$calledClass];
        if (is_callable($definition)) {
            $reflection = new \ReflectionFunction($definition);
            $returnType = $reflection->getReturnType();
            if ($returnType && $returnType instanceof \ReflectionNamedType) {
                $calledClass = $returnType->getName();
            } else {
                unset($calledClass);
            }
        } elseif (isset($definition['class'])) {
            $calledClass = $definition['class'];
        }
    }

    if (isset($calledClass) && get_class($widget) !== $calledClass) {
        throw new InvalidCallException('Expecting end() of ' . get_class($widget) . ', found ' . $calledClass);
    }

    /* @var $widget Widget */
    if ($widget->beforeRun()) {
        $result = $widget->run();
        $result = $widget->afterRun($result);
        echo $result;
    }

    return $widget;
}

but using self::$resolvedClasses seems more straightforward.