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

feat(dav): Emit a typed event to deprecate OCA\DAV\Connector\Sabre::addPlugin #39305

Merged
merged 1 commit into from Jul 18, 2023
Merged

feat(dav): Emit a typed event to deprecate OCA\DAV\Connector\Sabre::addPlugin #39305

merged 1 commit into from Jul 18, 2023

Conversation

nickvergessen
Copy link
Member

Checklist

@nickvergessen nickvergessen added enhancement 3. to review Waiting for reviews feature: dav pending documentation This pull request needs an associated documentation update labels Jul 11, 2023
@nickvergessen nickvergessen added this to the Nextcloud 28 milestone Jul 11, 2023
@nickvergessen nickvergessen requested review from ChristophWurst, come-nc and a team July 11, 2023 09:56
@nickvergessen nickvergessen self-assigned this Jul 11, 2023
@nickvergessen nickvergessen requested review from ArtificialOwl and icewind1991 and removed request for a team July 11, 2023 09:56
@nickvergessen nickvergessen mentioned this pull request Jul 11, 2023
Merged
5 tasks
@nickvergessen nickvergessen force-pushed the bugfix/noid/sabre-plugin-events branch from 8245c21 to f2edad2 Compare July 14, 2023 09:43
@nickvergessen
feat(dav): Emit a typed event to deprecate OCA\DAV\Connector\Sabre::a…
72667a6 
…ddPlugin

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/sabre-plugin-events branch from f2edad2 to 72667a6 Compare July 17, 2023 14:06
@kesselb
Copy link
Contributor

kesselb commented Jul 18, 2023

Super nitpick suggestion: Rename SabrePluginAddEvent to SabrePluginAddPluginEvent

To use the same "pattern" as for SabrePluginAuthInitEvent ;)

Event Class
OCA\DAV\Connector\Sabre::authInit SabrePluginAuthInitEvent
OCA\DAV\Connector\Sabre::addPlugin SabrePluginAddPluginEvent

kesselb
kesselb reviewed Jul 18, 2023
View reviewed changes
apps/dav/appinfo/v1/webdav.php
// allow setup of additional plugins
$event = new \OCP\SabrePluginEvent($server);
$dispatcher->dispatch('OCA\DAV\Connector\Sabre::addPlugin', $event);
$event = new \OCA\DAV\Events\SabrePluginAddEvent($server);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$event = new \OCA\DAV\Events\SabrePluginAddEvent($server);
$typedEvent = new \OCA\DAV\Events\SabrePluginAddEvent($server);

apps/dav/appinfo/v1/webdav.php
// allow setup of additional plugins
$event = new \OCP\SabrePluginEvent($server);
$dispatcher->dispatch('OCA\DAV\Connector\Sabre::addPlugin', $event);
$event = new \OCA\DAV\Events\SabrePluginAddEvent($server);
$dispatcher->dispatchTyped($event);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$dispatcher->dispatchTyped($event);
$dispatcher->dispatchTyped($typedEvent);

@nickvergessen
Copy link
Member Author

SabrePluginAddPluginEvent

I'll pass on this one with: https://docs.nextcloud.com/server/latest/developer_manual/basics/events.html#naming-scheme

The name should reflect the subject and the actions. Suffixing event classes with Event makes it easier to recognize their purpose.

<subject><action>Event => <SabrePlugin><Add>Event 🤷🏼

@nickvergessen nickvergessen merged commit 8e90cc0 into master Jul 18, 2023
36 of 37 checks passed
@nickvergessen nickvergessen deleted the bugfix/noid/sabre-plugin-events branch July 18, 2023 13:30
@nickvergessen nickvergessen mentioned this pull request Jul 28, 2023
Merged
15 tasks
@nickvergessen
Copy link
Member Author

Docs in nextcloud/documentation#10858

@nickvergessen nickvergessen removed the pending documentation This pull request needs an associated documentation update label Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kesselb kesselb kesselb approved these changes

@come-nc come-nc come-nc approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl was automatically assigned from nextcloud/server-backend

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 was automatically assigned from nextcloud/server-backend

@tcitworld tcitworld Awaiting requested review from tcitworld

Assignees

@nickvergessen nickvergessen

Labels
3. to review Waiting for reviews enhancement feature: dav
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nickvergessen @kesselb @tcitworld @come-nc