Add generic annotations to EventInterface

[villermen]

Q	A
Documentation	yes
Bugfix	no
BC Break	no
New Feature	no
RFC	no
QA	no

Description

It would be cool if you could somehow hint at the types of an event's target and params that IDEs and static code checking tools can work with. I've added a rough outline of what I mean in the PR using PHPStan-targeted annotations.

This way you can define the target and parameter types of an event in one go instead of doing it on each line the event is accessed:

public function onSomeEvent(EventInterface $event): void
{
    /** @var Element|null $element */
    $element = $event->getTarget();
    /** @var array{foo?: string} */
    $params = $event->getParams();
    
    $this->doSomethingWithFoo($params['foo']); // IDE can provide autocompletion and type recognition for "foo".
}

becomes

/**
 * @param EventInterface<Element|null, array{foo?: string}>
 */
public function onSomeEvent(EventInterface $event): void
{    
    $this->doSomethingWithFoo($event->getParams()['foo']); // IDE can provide autocompletion and type recognition for "foo".
}

This leaves the type annotation code in the main docblock of the method.

Of course you could also create an actual native-class for each event. That's more type-safe, but creates a lot of additional overhead. I can totally understand if that's the desired approach too, so take this is a suggestion only =)

[Ocramius]

Please also take a look at the CS failures: they are quite noisy :-D

[villermen]

Took care of the checks =)

Looks like Event correctly picks up on the variables passed in __construct(), setTarget() and setParams().

I was sadly unable to extend the template when calling setParam(). That ultimately leads to getParams() and getParam() only being able to check on the type specified or inferred from setParams(). So I'm not entirely happy with the result but it's the best I could muster.

[Ocramius]

Ok, now comes the hard part: testing this :D

In practice, you added a lot of type bindings to allow for better ergonomics around Event and EventInterface, but we don't know if they will work in the wild.

My suggestion is to have the types be verified by a directory checked by psalm.xml, with examples such as:

/**
 * @param EventInterface<MyObject, array<non-empty-string, 1|2|3>> $e
 * @return array{
 *     MyObject,
 *     array<non-empty-string, 1|2|3>,
 *     1|2|3
 * }
 */
function foo(EventInterface $e): MyObject {
    return [
        $e->getTarget(),
        $e->getParams(),
        $e->getParam('foo')
    ];
}

The above, but for various examples (constructor call, target change, params change, etc.)

[Ocramius]

Perhaps just drop TParams|null, and only keep TParams? That would fix this type binding, IMO, and remove the suppression on protected $params

[villermen]

I think that would be better yeah, paired with the reversal of the ctor null change.

The suppression for protected $params is because array<empty, empty> cannot be assigned to TParams . There seems to be no way to infer an empty array for TParams when the ctor receives null as the third argument.

I could fix that with $this->setParams($params ?? []), but that would change behavior when someone has extended Event without calling the parent's ctor. So I figured the suppression was a better solution.

---

Removing the $params = [] confuses Psalm because the default type for new Event() will become Event<null, mixed> which is not accepted by the likes of EventManager::triggerEvent().

Removing |null will cause a static error in the ctor because it thinks the type can never be null anymore.

[villermen]

I've added a check that verifies the intended behavior: https://github.com/laminas/laminas-eventmanager/pull/32/files#diff-89d6387e68462753db6f5bd8f489601c82db2d8af81c02ff4e27f868e9386dd8R10-R28

[Ocramius]

Just tracking this TODO

[Ocramius]

BTW, vimeo/psalm:^5 now installed (if you rebase).

It will either help or hurt :D

[internalsystemerror]

@villermen please address the comments made by @Ocramius, other than those, looks good 👍

[gsteel]

I'd prefer to see the psalm tests in a subdirectory of /test/ - that way, autoloading is already setup, psalm & phpcs runs and files are export ignored - not a blocker though.

[villermen]

I'd prefer to see the psalm tests in a subdirectory of /test/ [..]

@gsteel Actually agreed with you there, but then I noticed "benchmarks/". That's also a testing directory separate from "test/" so I stuck to that pattern. Ended up being happier with this situation because these classes are not necessarily tests that have to be run by PHPUnit. Will correct when you still prefer adding below "test/".

[gsteel]

I'd prefer to see the psalm tests in a subdirectory of /test/ [..]

Will correct when you still prefer adding below "test/".

I don't have strong opinions on this @villermen - it's all good as long as it's all export ignored and subject to the same CS rules etc 👍

[Ocramius]

Hot damn the tests on this are good! Thanks @villermen!

I added a couple more feedback points around verifying psalm-this-out, mostly because I got burnt by laminas/laminas-stdlib#78

If those are handled, we 🚢

[Ocramius]

Good enough though 👍

[Ocramius]

This is still kinda weird: fixable somehow? 🤔

See my reverts from laminas/laminas-stdlib#78

[Ocramius]

I see that you opted for &static: think that will suffice? 🤔

[villermen]

think that will suffice? 🤔

@Ocramius I think it does! After CheckEvent::setParams() or CheckEvent::setTarget() are called the type will become Event<NewTTarget, NewTParams&CheckEvent when using either &$this or &static.

I've also looked into getting it to work without this-out, but it become very cumbersome to deal with then. Ctor inference sets a type that's too narrow, setters can't use template types. The best we can do in that case is defining the templates and using them on the getters.

[Ocramius]

Let's roll with this and see how it goes: it's a massive improvement, and we all learned a lot from it, so it's time to test it on the field 👍

[Ocramius]

Thanks @villermen!