yiisoft / event-dispatcher

PSR-14 event dispatcher
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
68 stars 22 forks source link

Why does attach function of Provider become protected? #22

Closed lubobill1990 closed 4 years ago

lubobill1990 commented 4 years ago

https://github.com/yiisoft/event-dispatcher/commit/680194bad114f1516d3ca9f8ea52807992ccc507#r38900256

What steps will reproduce the problem?

n/a

What is the expected result?

The attach function should be public anyway.

What do you get instead?

I can't call it any more from my project code.

Additional info

Q A
Version 1.0.?
PHP version N/A
Operating system N/A
samdark commented 4 years ago

That was done by @yiiliveext on purpose to deny adding event handlers at runtime. It still can be done via friendly classes (the ones extending from AbstractProvideConfigurator).

samdark commented 4 years ago

Do you have a use case where it's not desirable?

yiiliveext commented 4 years ago

See https://github.com/yiisoft/yii-web/blob/master/src/Config/EventConfigurator.php and the example of usage https://github.com/yiisoft/yii-demo/blob/master/public/index.php#L23

lubobill1990 commented 4 years ago

@yiiliveext I'm confused by the example, why the attach method can be called here if it's protected in Yiisoft\EventDispatcher\Provider\Provider?

https://github.com/yiisoft/yii-web/blob/2f11fc5453e1d1a4dd9c09dd7d9b69332a888fb8/src/Config/EventConfigurator.php#L41

lubobill1990 commented 4 years ago

That was done by @yiiliveext on purpose to deny adding event handlers at runtime. It still can be done via friendly classes (the ones extending from AbstractProvideConfigurator).

I regard Yiisoft\EventDispatcher\Provider\Provider can be used independently in any project, if the attach function is protected and Provider is final, how can I use the Provider class?

My use case is,

  1. register the Yiisoft\EventDispatcher\Provider\Provider as singleton at bootstrap.php
  2. add a load.php to the same directory of composer.json of my project composer packages
  3. in my project root package, register "bootstrap": "ZhiShiQ\\Yii\\AppLoader" to composer.json at extra entry
  4. call those load.php in the AppLoader
  5. inject Provider to load.php, and listen to events for that package

With this method, I can encapsulate event listening code inside the package.

samdark commented 4 years ago

I'm confused by the example, why the attach method can be called here if it's protected in Yiisoft\EventDispatcher\Provider\Provider?

Because it's a friendly class extended from the same base abstract class.

lubobill1990 commented 4 years ago

So I have to first make a class extending AbstractProvideConfigurator, then am able to call attach in that class. It's too sinuous and preventing people from using this... We should have a balance between standard and flexibility. And why it's so important to deny adding event handlers at runtime? To support swoole?

samdark commented 4 years ago

In general, attaching events in runtime (except initialization) usually results in unpredictable and problematic behavior of the code so that was an attempt to prevent people doing potentially harmful thing by allowing to attach events from config only.

I totally agree that it's way less straightforward during initialization because of that but I'm afraid that if we'll not disallow doing it explicitly there will be lots of code doing exactly that even if we'll state in the guide that it should not be done.

lubobill1990 commented 4 years ago

One of the purposes that we split yii2 to packages and develop yii3 is we hope every part of yii3 is valuable, people can adopt one sub package to their project rather than have to use the whole ecosystem. Currently, it seems that we change the event dispatcher package because of the way we use in other yii3 packages.

Even though people may make mistakes if they attach events after initialization, which can be debugged easily and we can also document it to help, we can't totally disable them to do, because there are indeed such scenarios, and force user to extend AbstractProvideConfigurator restricted their implementation too much, and make this package unpopular. (I think the current package is fairly good, potential to be popular, light wight and usable.)

In my previous example, I event don't use a class to bind the events, but event listenings are all defined in a load.php which is called when application bootstrapped and is a standard in my project.

I hope the attach function can be made public again.

yiiliveext commented 4 years ago

So I have to first make a class extending AbstractProvideConfigurator, then am able to call attach in that class. It's too sinuous and preventing people from using this... We should have a balance between standard and flexibility. And why it's so important to deny adding event handlers at runtime? To support swoole?

If you want to make this method public, you can do something like that

class MyProvider extends Provider
{
    public function attach(callable $listener, string $eventClassName = ''): void
    {
        parent::attach($listener, $eventClassName);
    }
}

Only seven lines of code and you have Provider with the public attach() method.

lubobill1990 commented 4 years ago

So I have to first make a class extending AbstractProvideConfigurator, then am able to call attach in that class. It's too sinuous and preventing people from using this... We should have a balance between standard and flexibility. And why it's so important to deny adding event handlers at runtime? To support swoole?

If you want to make this method public, you can do something like that

class MyProvider extends Provider
{
    public function attach(callable $listener, string $eventClassName = ''): void
    {
        parent::attach($listener, $eventClassName);
    }
}

Only seven lines of code and you have Provider with the public attach() method.

But Provider is final which can't be extended.

lubobill1990 commented 4 years ago

Any news?

In fact, before this, I never knew protected can be called in sibling class. In most resources, portected is described as accessible from children and ancestors.

I believe most other developers will regard it as strange trick.

I searched and find this: https://wiki.php.net/rfc/protectedlookup

It seems it's somewhat a bug.

samdark commented 4 years ago

https://twitter.com/sam_dark/status/1261435140154314759

samdark commented 4 years ago

@lubobill1990 so nope, the "friendly classes" behavior we're using is normal won't be removed from PHP.

samdark commented 4 years ago

29

samdark commented 4 years ago

That's still controversial but @romkatsu finally added it to readme :)

lubobill1990 commented 4 years ago

No problem, I worked around this problem by replacing Dispatcher and Provider with customized Dispatcher and Provider which extends from AbstractProviderConfigurator and works as proxy to original Dispatcher and Provider.

samdark commented 4 years ago

Decided to keep it as is for a while.

samdark commented 4 years ago

Trying alternative approach in #22.