yiisoft / widget

Widgets are reusable building blocks used to create complex and configurable user interface elements in an object-oriented fashion
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
25 stars 6 forks source link

The `Widget` class don't work directly without initialization #3

Closed pchapl closed 4 years ago

pchapl commented 5 years ago

What steps will reproduce the problem?

AnyWidget::widget(), where AnyWidget's ancestor is Yiisoft\Widget\Widget

What is the expected result?

Widget instantiating.

What do you get instead?

TypeError
Argument 1 passed to Yiisoft\Widget\Widget::__construct() must implement interface Psr\EventDispatcher\EventDispatcherInterface, null given, called in view/src/Widget/Widget.php on line 109

Additional info

There is $this->container->get(Widget::class); in view/tests/TestCase.php. After this initialization AnyWidget::widget() works properly. Reason: \Yiisoft\Widget\Widget::__construct sets dependencies to static fields.

Q A
Version master [8d9f5ea2770835c05b49281a6ee53bbf24428a1d]
PHP version 7.3
Operating system *nix
terabytesoftw commented 5 years ago

Hi, you must configure widget in config/web:

<?php

use Psr\Container\ContainerInterface;
use Psr\EventDispatcher\EventDispatcherInterface;
use Psr\EventDispatcher\ListenerProviderInterface;
use Psr\Log\LoggerInterface;
use Yiisoft\Aliases\Aliases;
use Yiisoft\Asset\AssetConverter;
use Yiisoft\Asset\AssetManager;
use Yiisoft\EventDispatcher\Dispatcher;
use Yiisoft\EventDispatcher\Provider\Provider;
use Yiisoft\Factory\Definitions\Reference;
use Yiisoft\Log\Logger;
use Yiisoft\View\Theme;
use Yiisoft\View\View;
use Yiisoft\View\WebView;
use Yiisoft\Widget\Widget;

return [
    ContainerInterface::class => function (ContainerInterface $container) {
        return $container;
    },

    Aliases::class => [
        '@root' => dirname(__DIR__, 1),
        '@public' => '@root/tests/public',
        '@basePath' => '@public/assets',
        '@baseUrl'  => '/baseUrl',
        '@converter' => '@public/assetconverter',
        '@npm' => '@root/node_modules',
        '@view' => '@public/view',
        '@web' => '@baseUrl',
        '@testSourcePath' => '@public/assetsources'
    ],

    AssetConverter::class => [
        '__class' => AssetConverter::class,
        '__construct()' => [
            Reference::to(Aliases::class),
            Reference::to(LoggerInterface::class)
        ]
    ],

    AssetManager::class => [
        '__class' => AssetManager::class,
        '__construct()' => [
            Reference::to(Aliases::class),
            Reference::to(LoggerInterface::class)
        ],
        'setBasePath' => ['@basePath'],
        'setBaseUrl'  => ['@baseUrl'],
    ],

    ListenerProviderInterface::class => [
        '__class' => Provider::class,
    ],

    EventDispatcherInterface::class => [
        '__class' => Dispatcher::class,
        '__construct()' => [
           'listenerProvider' => Reference::to(ListenerProviderInterface::class)
        ],
    ],

    LoggerInterface::class => [
        '__class' => Logger::class,
        '__construct()' => [
            'targets' => [],
        ],
    ],

    Theme::class => [
        '__class' => Theme::class,
    ],

    WebView::class => function (ContainerInterface $container) {
        $aliases = $container->get(Aliases::class);
        $eventDispatcher = $container->get(EventDispatcherInterface::class);
        $theme = $container->get(Theme::class);
        $logger = $container->get(LoggerInterface::class);

        return new WebView($aliases->get('@view'), $theme, $eventDispatcher, $logger);
    },

    View::class => function (ContainerInterface $container) {
        $aliases = $container->get(Aliases::class);
        $eventDispatcher = $container->get(EventDispatcherInterface::class);
        $theme = $container->get(Theme::class);
        $logger = $container->get(LoggerInterface::class);

        return new View($aliases->get('@view'), $theme, $eventDispatcher, $logger);
    },

    Widget::class => [
        '__class' => Widget::class,
        '__construct()' => [
            Reference::to(EventDispatcherInterface::class),
            Reference::to(WebView::class),
        ]
    ],
];
rob006 commented 5 years ago

That does not look right. This is not a documentation problem, this is design issue - Widget should not rely on static fields - widgets stack and dependency injection should be implement at view level (like $this->widget(MyWidget::class, /* ... */) instead of MyWidget::widget(/* ... */)).

Also this:

https://github.com/yiisoft/view/blob/8d9f5ea2770835c05b49281a6ee53bbf24428a1d/src/Widget/Widget.php#L109

Looks like really naive assumption. What if I change constructor of my widget to contain additional dependencies?

terabytesoftw commented 5 years ago

I understand the problem rather than injecting the dependencies into the constructor we can create two setters that solve the problem.

samdark commented 5 years ago

@rob006 so would the following work?

public function widget(string $className, array $constructorArguments = [])
{
    return $this->container->get($className, ['__construct()' => $constructorArguments]);
}

Syntax would be:

<?= $this->widget(Menu::class)
    ->encodeLabels(true)
    ->items([
        [
            'encode' => false,
            'label'  => '<span class="glyphicon glyphicon-user"></span> Users',
            'url'    => '#',
        ],
        [
            'encode' => true,
            'label'  => 'Authors & Publications',
            'url'    => '#',
        ],
    ]) ?>
rob006 commented 5 years ago

View has dependency on DI container instance . That's not good but I see no other way, do you?

We could require to pass all dependencies explicitly in view, but that could be quite annoying.

widget($className, ['42']) implementation is something like

Is there any reason to not use new MyWidget() directly? It at least give you some hints from IDE.

pchapl commented 5 years ago

I'm playing around right now. Made something like @samdark 's suggestion, but without emplicit configuration. Going to create pr soon.

samdark commented 5 years ago

Is there any reason to not use new MyWidget() directly? It at least give you some hints from IDE.

Only to hide passing container around. Actually for current widgets passing $this should be enough.

terabytesoftw commented 5 years ago

I have thought of a solution that does not need to inject the container in View(), nor register all the widgets in the container, I have converted the widget class from static to instance class.

in Layouts:

$nav = new Nav($this);
$navBar = new NavBar($this);

<?php $navBar
    ->brandLabel($params['app.name'])
    ->brandUrl($params['app.home.url'])
    ->options([
        'class' => 'navbar navbar-dark bg-dark navbar-expand-lg text-white',
    ])
    ->begin();

    echo $nav
        ->activateParents(true)
        ->currentPath($currentPath)
        ->items($menuItems)
        ->options(['class' => 'navbar-nav float-right ml-auto']);

$navBar->end(); ?>

in WebView:

public function getEventDispatcher(): EventDispatcherInterface
{
    return $this->eventDispatcher;
}

in Widgets:

public function __construct(WebView $webView)
{
    $this->eventDispatcher = $webView->getEventDispatcher();
    $this->webView = $webView;
}

public function begin()
{
    $this->init();
}

public function end()
{
    if ($this->beforeRun()) {
        $result = $this->run();
        $result = $this->afterRun($result);
        echo $result;
}

    return $this;
}

public function run(): string
{
   $out = '';

   if ($this->beforeRun()) {
       $result = $this->getContent();
       $out = $this->afterRun($result);
   }

   return $out;
}

@rob006 Some questions:

1.- What is the advantage of implementing beginWidget() and endWidget() at the view level ? 2.- How to use $this for each widget, for example if i use nav(), navbar(), I would have $this->nav and $this->navbar and so on for each one is what i can understand from https: // forum. yiiframework.com/t/using-functions-for-html-generation/126814/9?u=rob006

With proper guidance I think we could all be satisfied with a new implementation.

rob006 commented 5 years ago

1.- What is the advantage of implementing beginWidget() and endWidget() at the view level ?

  1. Syntax sugar?
  2. Additional guard provided by widgets stack - it will warn you if you try call begin() and end() for multiple widgets in incorrect order.
  3. DI. If dependencies will be resolved automatically, it will simplify using widgets and upgrading process - if widget will add one additional dependency to constructor, you don't need to fix all usages of widget to pass new dependency explicitly.

2.- How to use $this for each widget, for example if i use nav(), navbar(), I would have $this->nav and $this->navbar and so on for each one is what i can understand from https: // forum. yiiframework.com/t/using-functions-for-html-generation/126814/9?u=rob006

I don't understand the question.

terabytesoftw commented 5 years ago

@rob006 If I understand your example it would be:

<?php $this->beginWidget(NavBar::class)
    ->brandLabel($params['app.name'])
    ->brandUrl($params['app.home.url'])
    ->options([
        'class' => 'navbar navbar-dark bg-dark navbar-expand-lg text-white',
    ]);

    echo $this->widget(Nav::class)
        ->activateParents(true)
        ->currentPath($currentPath)
        ->items($menuItems)
        ->options(['class' => 'navbar-nav float-right ml-auto']);

$this->endWidget(NavBar::class); ?>

Of course if I do it in WebView() all the dependencies would have them available and I would not need to inject anything into the constructor.

I had not seen the truth in this way the syntax is more readable, if this is the idea I think I can develop it.

rob006 commented 5 years ago

More like

<?= $this->beginWidget(NavBar::class, [], static function (NavBar $widget) use ($params) {
    // configuration via setters after constructor
    return $widget->brandLabel($params['app.name'])
        ->brandUrl($params['app.home.url'])
        ->options([
            'class' => 'navbar navbar-dark bg-dark navbar-expand-lg text-white',
        ]);
}) ?>

<?= $this->widget(Nav::class, [
    // configuration via $config array in constructor
    'config' => [
        'activateParent' => true,
        'currentPath' => $currentPath,
        'items' => $menuItems,
        'options' => ['class' => 'navbar-nav float-right ml-auto'],
    ],
]) ?>

<?= $this->endWidget(NavBar::class) ?>

I have no idea how to handle constructor here. Something like named parameters may be useful here, so only specific dependencies could be provided explicitly and rest would be resolved by container.

terabytesoftw commented 5 years ago

Thinking a little, the library view, should only allow to process html, css, js, that should be enough to design any software, currently the package is coupled to assets and widgets, which should not be convenient, if I want to use only in the html, css and js views will always have all those preloaded packages without using them, the view should be independent and all assets, helpers and widgets should be injected into the view, but not that the view resolves dependencies and processes them are my two cents.

fcaldarelli commented 5 years ago

What about reference to widget stack when we have subviews? I mean, if we have a single widget stack for widgets in different subviews, how can manage It?

I don't know if we have a single or multiple WebView objects during rendering.

Finally, this package should be usable independently from Yii framework, so "widgets" property would not make sense in other contexts.