Public API's GenericEvent replacement #17650
Conversation
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
| */ | ||
| public function offsetGet($offset) { | ||
| if ($this->hasArgument($offset)) { | ||
| unset($this->arguments[$offset]); |
There was a problem hiding this comment.
| unset($this->arguments[$offset]); | |
| return $this->arguments[$offset]; |
| * | ||
| * @since 18.0.0 | ||
| */ | ||
| public function setArguments(array $args = []): GenericEvent { |
There was a problem hiding this comment.
this allows integer keys, while setArgument doesnt
I think we should unify this?
| * @link https://php.net/manual/en/arrayaccess.offsetset.php | ||
| * @since 18.0.0 | ||
| */ | ||
| public function offsetSet($offset, $value): void { |
There was a problem hiding this comment.
this explodes when offset is not a string, which you enforce on setArgument but this method here allows.
|
If many of the changes are breaking, wouldn't it make sense to migrate to |
|
https://github.com/search?q=org%3Anextcloud+%22GenericEvent%22&type=Code for the code we might have to touch 😢 |
Could do… it makes the constructor case messier. It is not terrible though. However it would be cumbersone to tell whether the thrown events make use of the available features of the GenericEvent or not. I'd prefer to keep it separated.
About half of it is addressed with this PR already. Then there is of course a number of potential apps not on github. Rest assured I am uneasy with this as well and still carry a portion of doubts. The question is, how can we do a smooth transition? Since a lot of code directly relies on Symfony's GenericEvent, we cannot simply put an interface on top or phase it out. What we could do differently however is to sent the events twice, with the new and the old implementation. And then we are able to remove the old GenericEvent at a later point, 3 years from 18 or what. The SymfonyAdapter already logs deprecation messages, when the old event type is being used. Sadly, we cannot do it when listeners are addressed, unless patching Symfony's event dispatcher… not sure this is a good idea. All in all that's probably the better thing. |
|
closing in favor of #17834 |
In some (but not every) place we use Symfony's
GenericEventclass. It depends on their deprecatedEvent. We have made the switch to use the newEventalready. In itself it is not contradicting (unless code checks for old or new Event class).In order to complete the transition, I added a new GenericEvent in the OCP space, which extends the new Event class, but is also compatible signature-wise to Symfony's
GenericEvent.Thus, adjusting is easy by replacing the namespace. And yet it is a hard break. Going full to new GenericEvent means that any code or app that listens to the one of the events and type-hints the old
GenericEvent, will run into an error condition. Since we need to deal with classes, not interfaces, there is no way around this sadly.Thus, if we accept this change, we need to touch any other app that is also outside of this repository and communicate as critical change. I do not like doing it, but it is a good opportunity to go for a baseline of our events and gain some consistency.
Thoughts?