Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Public API's GenericEvent replacement (take 2) #17834

Merged
merged 2 commits into from
Nov 27, 2019
Merged

Public API's GenericEvent replacement (take 2) #17834

merged 2 commits into from
Nov 27, 2019

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Nov 6, 2019 •
edited
Loading

replacement of #17650

The approach above is not really awesome, because it causes breaks and kills backwards compatibility.

The approach now is to

  • Still add an own GenericEvent class in OCP
    • (combining it with Event does not work as it breaks current implementations that have some methods with a different signature)
  • new Events should straight use Event, or less ideally this GenericEvent, anyway not Symfony's old ones
  • old Events should throw a second event using the new class and phase out the old event as we do we regular deprecations
    • this is not within scope of this PR and can be handled invidiually. We should do it for the server parts sooner however.

@blizzz blizzz added this to the Nextcloud 18 milestone Nov 6, 2019
@blizzz blizzz mentioned this pull request Nov 6, 2019
Closed
@ChristophWurst
Copy link
Member

new Events should straight use this GenericEvent, not Symfony's old one

New events should subclass our Event :)

@blizzz
Copy link
Member Author

blizzz commented Nov 6, 2019

new Events should straight use this GenericEvent, not Symfony's old one

New events should subclass our Event :)

updated the intro

juliusknorr
juliusknorr reviewed Nov 19, 2019
View reviewed changes
lib/public/EventDispatcher/GenericEvent.php
* \OCP\EventDispatcher\Event
*
* @package OCP\EventDispatcher
* @since 18.0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about deprecating it right away?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then, we can also skip this entirely

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still makes it easier to refactor step by step that way I'd say.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What in general is problematic with private events is that they do not follow an API. So you can put there some magic in it, and no app can reliably do anything with it, because there is no stable description of it. This GenericEvent offers some access methods. So while you still do not know what exactly is behind, at least there a standardized access methods to get context out of that event. Therefore, I wouldn't really want to deprecate it at all, tbh.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, also fine by me then.

@juliusknorr juliusknorr mentioned this pull request Nov 19, 2019
Merged
1 task
@ChristophWurst ChristophWurst mentioned this pull request Nov 19, 2019
Merged
@blizzz
Copy link
Member Author

blizzz commented Nov 20, 2019

@ChristophWurst what about you? :)

ChristophWurst
ChristophWurst approved these changes Nov 20, 2019
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoooooops I thought I already pressed the button. Sorry :)

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 20, 2019
@blizzz blizzz force-pushed the enh/noid/generic-event-replacement-tk2 branch 2 times, most recently from 6326091 to f04dbc5 Compare November 22, 2019 13:21
@blizzz blizzz mentioned this pull request Nov 22, 2019
Merged
5 tasks
@blizzz
Copy link
Member Author

blizzz commented Nov 22, 2019

i'll need to touch tests → next week

@blizzz blizzz force-pushed the enh/noid/generic-event-replacement-tk2 branch 2 times, most recently from 5c0e179 to ea4bee6 Compare November 25, 2019 16:24
@blizzz blizzz force-pushed the enh/noid/generic-event-replacement-tk2 branch from ea4bee6 to 0727d0a Compare November 26, 2019 09:28
@blizzz
Copy link
Member Author

blizzz commented Nov 26, 2019

great, test pass locally :-/

juliusknorr
juliusknorr reviewed Nov 26, 2019
View reviewed changes
tests/lib/Files/Node/HookConnectorTest.php Outdated
@@ -237,7 +241,7 @@ public function testPostDeleteMeta() {
$dispatcherCalled = false;
/** @var Node $dispatcherNode */
$dispatcherNode = null;
$this->eventDispatcher->addListener('\OCP\Files::postDelete', function (GenericEvent $event) use (&$dispatcherCalled, &$dispatcherNode) {
$this->eventDispatcher->addListener('\OCP\Files::postDelete', function (APIGenericEvent $event) use (&$dispatcherCalled, &$dispatcherNode) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no APIGenericEvent dispatched but a symfony GenericEvent, so the type hint seems to cause the test failures.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes them locally but tests also only fail for me on a clean checkout when running all tests though autotest.sh, not when running them individually

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have not seen your comment since, but yeah, with a fresh checkout i could reproduce this, too, and found the culprit quickly. Thanks still :)

@blizzz
Files first to use own GenericEvent
fc16b09 
* those are added to 18 only anyway :)

Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz blizzz force-pushed the enh/noid/generic-event-replacement-tk2 branch from 27063c1 to fc16b09 Compare November 26, 2019 13:49
@blizzz blizzz merged commit d2f9deb into master Nov 27, 2019
@blizzz blizzz deleted the enh/noid/generic-event-replacement-tk2 branch November 27, 2019 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliusknorr juliusknorr juliusknorr approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@icewind1991 icewind1991 Awaiting requested review from icewind1991

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement technical debt
Projects
None yet
Milestone
Nextcloud 18.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@blizzz @ChristophWurst @juliusknorr