event-manager icon indicating copy to clipboard operation
event-manager copied to clipboard

Scalar typehints

Open vojta001 opened this issue 3 years ago • 3 comments

Why don't you type hint scalar args? E.g.

https://github.com/doctrine/event-manager/blob/41370af6a30faa9dc0368c4a6814d596e81aba7f/lib/Doctrine/Common/EventManager.php#L32

$eventArgs is type hinted, but $eventName is not. Since the minimum supported version is PHP 7.1

https://github.com/doctrine/event-manager/blob/37b7882d2b9e6aed5a766ac43450db462abd5ee9/composer.json#L23

there should be no compatibility issues.

If the answer is just because nobody has done it yet, I am ready to send a PR. It would make the life of your users much easier; they cannot put the hint to their subclasses right now, because that would narrow down the allowed types. On the other hand, if they want to make it broader later, they will still be allowed to do so.

vojta001 avatar Jul 29 '20 14:07 vojta001

On the other hand, if they want to make it broader later, they will still be allowed to do so.

That's only possible if you use php 7.2 though, which means adding the type declarations would be a BC break

greg0ire avatar Jul 29 '20 18:07 greg0ire

Ah, I see, i forgot about it.

However, PHP 7.1 is EOL. Would't it be just fine to raise the minimal supported PHP version and then type hint?

vojta001 avatar Jul 30 '20 15:07 vojta001

It think it would, please send a PR :) Make sure to only add parameter type declarations, otherwise it will not be BC.

greg0ire avatar Jul 30 '20 16:07 greg0ire

Will be fixed in 2.0.

derrabus avatar Oct 12 '22 21:10 derrabus