base\Widget end() method throws Invalid Call exception if i override widget class in definition

[papppeter]

What steps will reproduce the problem?

Add Pjax to definitions

\yii\widgets\Pjax::class => [
    'class' => \system\widgets\Pjax::class
]

What is the expected result?

\system\widgets\Pjax::class class will be used insted of \yii\widgets\Pjax::class

What do you get instead?

Expecting end() of system\widgets\Pjax, found yii\widgets\Pjax

Additional info

Q	A
Yii version	2.0.43
PHP version	7.4.3
Operating system	Windows

Proposal:

public static function end() 
{

    if (!empty(self::$stack)) {
        $widget = array_pop(self::$stack);

        $calledClass = get_called_class();
        if(Yii::$container->has($calledClass) && isset(Yii::$container->getDefinitions()[$calledClass]['class'])) {
            $calledClass = Yii::$container->getDefinitions()[$calledClass]['class'];
        }

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

            return $widget;
        }

        throw new InvalidCallException('Expecting end() of ' . get_class($widget) . ', found ' . get_called_class());
    }

    throw new InvalidCallException('Unexpected ' . get_called_class() . '::end() call. A matching begin() is not found.');
}

[samdark]

Interesting use-case. How would that affect performance?

[WinterSilence]

@papppeter DI work with objects, but this is static, you must use something like this:

use system\widgets\Pjax;
Pjax::begin();
...
Pjax::end();

[papppeter]

@papppeter DI work with objects, but this is static, you must use something like this:

use system\widgets\Pjax;
Pjax::begin();
...
Pjax::end();

Pjax::begin() works out of the box. It is goes trough DI. Only the end() part does not. So from my point of view it is handled incosistently.

It is really helpfull if i would like to rewrite some vendor CRUDs which utilize widgets.

my use case is:

• our projects relays on our vendor packages
• vendor need to be backward or/and all projects compatible (unfortunatelly we are not using versioning here - small team)
• we have modules in vendor which implements CRUDs
• we have one heavelly used theme widget Ibox (from inspinia theme) which was replaced with Box (from adminelte)
• with DI i could achive not to touch vendors and switch from Ibox to Box easily

I think widget DI would be a perfect addition to the framework.

@samdark I did not tested, but my solution is using basic php functions, i think it should not affect the performance. But if you are pozitive i could test it out.

[WinterSilence]

@papppeter

Pjax::begin() works out of the box.

I'm mean you can't call Yii::$container->get(\yii\widgets\Pjax::class)->begin();.
Can you post your view with Pjax?

[papppeter]

@papppeter

Pjax::begin() works out of the box.

I'm mean you can't call Yii::$container->get(\yii\widgets\Pjax::class)->begin();. Can you post your view with Pjax?

my code looks following:

<?php
use yii\widgets\Pjax;
?>
<?php $pjaxWidget = Pjax::begin(); ?>
...
<?php Pjax::end(); ?>

Error:

Expecting end() of modules\system\widgets\Pjax, found yii\widgets\Pjax

So begin part calls Yii::createObject -> modules\system\widgets\Pjax is used
End part does not take care of DI -> yii\widgets\Pjax is used

[WinterSilence]

@papppeter i'm think it's wrong way, because you call yii\widgets\Pjax::end();, not system\widgets\Pjax::end(); or [get_class($pjaxWidget), 'end']()

[papppeter]

@papppeter i'm think it's wrong way, because you call yii\widgets\Pjax::end();, not system\widgets\Pjax::end(); or [get_class($pjaxWidget), 'end']()

@WinterSilence as long code belongs to the project i accept your answer.

What if it is a vendor code? I don't have the possibility to change it. (i know i could overwrite it, but that would basically a copy paste of the old code)

[WinterSilence]

@papppeter you solution may create troubles at work in team: you hiddenly change functionality, IDE can't normally analyze code - your teammates must remember all your changes in project.

[papppeter]

@papppeter you solution may create troubles at work in team: you hiddenly change functionality, IDE can't normally analyze code - your teammates must remember all your changes in project.

@WinterSilence I do not agree with your statement. We have proper definitions.php in place, readmes etc. which tells my team how our system works. We are utilizing DI in our projects, which works perfectly everywhere but not with widgets.

IDE can normally analyze the code we still have only begin(), end() nothing changes.

So maybe you could explain a little bit more, with an example where do you see it will affect badly the code quality.

[papppeter]

Here is an example: i would like to modify widget autoIdPrefix if we have a matching pjax call. It think uuid would fit better if i would like to use auto id behavior. What would be your suggestion how should i do it?

For now i needed to do it with an event, copy some code over from pjax widget. (by the way it works perfeclty)

Nicer soultion would be to replace pjax widget, which uses my version how we change the behavior if requiresPjax (in this way i could achive much more)

Of course this relates a vendor package which i don't one a touch, or fork.

[WinterSilence]

@papppeter

We have proper definitions.php in place, readmes etc. which tells my team how our system works.

it will be nightmare for new teammates. https://sourcemaking.com/antipatterns/jumble

IDE can work fine

PhpDoc declare return type of Pjax::begin(); as static, but in you case it's not true

[papppeter]

@WinterSilence Please give me a good solution if I want to rewrite a widget in vendor how should I do it? Main scope is the vendor! (i agree with your suggestion to use the new class directly at project files)

And i think if begin() works differently from the end() than it is bug. At the begin part i am allowed to use DI, className::widget() will work with a DI. className::begin(), className::end() won't. This needs to be addressed somehow.

[WinterSilence]

@papppeter 1. split modules/packages: class definitions and views using that's classes
2. use class decorator/proxy or hints as class Yii extends BaseYii for views

[papppeter]

@papppeter 1. split modules/packages: class definitions and views using that's classes 2. use class decorator/proxy or hints as class Yii extends BaseYii for views

Can you please explain a little bit more. i am still in a vendor, would like to adjust a 3rd party code. Are you suggestion to copy the views i need my change?

Widget behaviour is not consistent still looks like a bug to me. What do you think?

[WinterSilence]

@papppeter good vendor packages don't contains views or they have solution to customize/replace it (as yii/bootstrap). can you show packages you are having problems with?

[papppeter]

@papppeter good vendor packages don't contains views or they have solution to customize/replace it (as yii/bootstrap). can you show packages you are having problems with?

I do know that you can replace all views. But it approche has cons and pros. In may case it has more cons. What if a new version of a package brings new views in?

Here some example repositories are using views (i cannot share my repositorys they are private but we are using views as well):
https://github.com/yiisoft/yii2-gii
https://github.com/luyadev/luya-module-cms
https://github.com/mdmsoft/yii2-admin
https://github.com/amnah/yii2-user
https://github.com/e282486518/yii2admin
https://github.com/liufee/cms
https://github.com/webtoolsnz/yii2-scheduler

The direction of the conversation I think is not going anywhere. Everyone uses the framework as they see fit. Maybe we need only to stick to my second question:

Widget behaviour is not consistent still looks like a bug to me. What do you think?

if begin() works differently from the end() than it is bug. At the begin part i am allowed to use DI, className::widget() will work with a DI. className::begin(), className::end() won't. This needs to be addressed somehow.

[WinterSilence]

@papppeter yes, as minimum, need replace if (get_class($widget) === get_called_class()) to if ($widget instanceof get_called_class()) in Widget::end()

[papppeter]

@papppeter yes, as minimum, need replace if (get_class($widget) === get_called_class()) to if ($widget instanceof get_called_class()) in Widget::end()

That would mostly fit. If i don't wan't to work with inherrintence then not. Still i would go with my suggestion. But i don't know will it be a performance issue or not. But i am happy to create a test on my side.

Do you think you could suggest to @samdark to mark this as a bug fix request?

[WinterSilence]

@papppeter that's problem is complex - what's do if you need create class C extending class A defined as class B extends A?

What if a new version of a package brings new views in?

abstract/flex code is grant it's re-usable. see yii2 themes for more info

@papppeter

or they have solution to customize/replace it

yiisoft/yii2-gii support custom templates and some packages in list too

[papppeter]

@papppeter that's problem is complex - what's do if you need create class C extending class A defined as class B extends A?

if we go with my solution, that would not be a problem. Basically my solution makes widget end() consistent to begin()

yiisoft/yii2-gii support custom templates and some packages in list too

i know that yii2 out of the box supports custom templates (theme), but admin apps do contain more 100 then views.

[WinterSilence]

@papppeter

admin apps do contain more 100 then views.

application is root package or your application inherit vendor applications? separated modules better than monolith application "all in"

[papppeter]

application is root package or your application inherit vendor applications? separated modules better than monolith application "all in"

no we are not inherit from vendor application we use it as a module and use it as is. We are heavily relying on module structure.

sorry i don't understand russian, but as structure we use similar.

[WinterSilence]

@papppeter

sorry i don't understand russian

google translate: https://elisdn-ru.translate.goog/blog/85/seo-service-on-yii2-reusing-of-modules?_x_tr_sl=ru&_x_tr_tl=en&_x_tr_hl=ru&_x_tr_pto=nui