TASK: Use "static" return type where needed

[robertlemke]

This change corrects return types to "static" where late static binding is used and corrects new static to new self in classes declared as final.

Resolves #309

[bwaidelich]

This change corrects return types to "static" where late static binding is used

Makes sense, thanks!

corrects new static to new self in classes declared as final.

This is dangerous IMO because proxy for classes the final is removed and new self() will actually return the _Original instance. And since Flow currently creates a proxy for every class that has no Proxy(false) annotation, that will most probably have side effects.

[robertlemke]

You're right, that's why I stopped using "self" in factory methods a long time ago. It's safe to use the actual class name though, as that will stay intact even when the class is proxied.

I pushed a new commit accordingly.

[robertlemke]

PS: I didn't change anything in fixture classes and for accessing constants (self::SOME_FOO).

[bwaidelich]

Mh.. I find using the class names rather "cumbersome" and PHPStorm complains about it. Is it any better than self?

Why don't we use a return type of static where we actually use new static() (i.e. in AbstractEventSourcedAggregateRoot::reconstituteFromEventStream() and keep self for the rest?

btw: I can confirm that the previous version broke my app

[robertlemke]

Mh.. I find using the class names rather "cumbersome" and PHPStorm complains about it. Is it any better than self?

For me it's just the contrary:

• PHPStorm complains if I use "return new static" in a final class
• PHPStorm does not complain if Iuse "return new TheClass" in that class

Why don't we use a return type of static where we actually use new static() (i.e. in AbstractEventSourcedAggregateRoot::reconstituteFromEventStream() and keep self for the rest?

We can do that, but in my case PHPStorm complains about it

[robertlemke]

[bwaidelich]

OK, alternative suggestion: We add a @Flow\Proxy(false) to all affected final classes and use new self() wdyt?

[bwaidelich]

We add a @Flow\Proxy(false) to all affected final classes

This would include the following classes if I'm correct::

• DefaultAppliedEventsStorage (but that currently still uses @Flow\Inject)
• EventListenerInvoker
• DeferEventPublisher (currently relies on Flow object lifecycle methods!)

all others already have the Proxy annotation

[robertlemke]

We add a @Flow\Proxy(false) to all affected final classes and use new self()

I'm not against it, but it feels a bit fragile for me, as you always need to be aware if proxies are disabled or not. You need to think too much for my taste. Not using self is a rule I can work with which doesn't make me think.

But just as you like, I don't have strong feelings for that.

[bwaidelich]

it feels a bit fragile for me, as you always need to be aware if proxies are disabled or not

Yes, that's right.. But IMO it's also fragile to let the proxy building dictate our style of coding. new ClassName feels like PHP 4 to me :D

But I also have no strong feelings about this, just a general tendency: In the light of #180 we should slowly get rid of Flow specific (or idiomatic) code in the core components of this library and rely on object management etc. only in some thin integration layer (or even package). Your PR made me aware that we still need to fix a couple of core classes. For example: DefaultAppliedEventsStorage has a static constructor forEventListener() but it also has the EntityManager injected. This is not a clean architecture.. Fortunately those are just a few cases AFAIK.

To cut a long story short: In this case I would opt for the minimal invasive solution to the problem you described in the issue. I created #311 not to mess with your PR – feel free to take over the changes :)

[robertlemke]

🤷🏻