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

Add LoadAdditionalScriptsEvent for files_sharing #21815

Merged
merged 1 commit into from
Jul 15, 2020
Merged

Add LoadAdditionalScriptsEvent for files_sharing #21815

merged 1 commit into from
Jul 15, 2020

Conversation

juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jul 13, 2020 •
edited by MorrisJobke
Loading

Move old the OCA\Files::loadAdditionalScripts and OCA\Files::loadAdditionalScripts::publicShareAuth events to our the public event API. An adapter is added as well to still dispatch the old events on the until the old ones are removed completely.

@juliusknorr juliusknorr added this to the Nextcloud 20 milestone Jul 13, 2020
@juliusknorr juliusknorr mentioned this pull request Jul 13, 2020
Merged
@MorrisJobke
Copy link
Member

I just noticed that you still use the newer old way. The newest one is the IEventDispatcher 🙈 Let's have a quick coordination chat.

@MorrisJobke
Copy link
Member

#14552 (comment)

MorrisJobke
MorrisJobke approved these changes Jul 13, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good and works 👍

@MorrisJobke MorrisJobke force-pushed the enh/noid/sharing-additional-scripts-event branch from 16ce985 to b329c95 Compare July 13, 2020 20:24
@MorrisJobke
Copy link
Member

Rebased and autosquashed 🚀

@MorrisJobke MorrisJobke force-pushed the enh/noid/sharing-additional-scripts-event branch from b329c95 to aa5e98a Compare July 13, 2020 20:30
@MorrisJobke
Copy link
Member

And I fixed the unit tests 🙈

This was referenced Jul 13, 2020
Merged
Merged
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Jul 14, 2020
ChristophWurst
ChristophWurst requested changes Jul 14, 2020
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.

A theoretical concern but it's easy to fix now than having another migration path for a public event that takes three years to get rid of.

apps/files_sharing/lib/Event/LoadAdditionalScriptsEvent.php Outdated Show resolved Hide resolved
@MorrisJobke MorrisJobke mentioned this pull request Jul 14, 2020
Merged
2 tasks
@juliusknorr juliusknorr force-pushed the enh/noid/sharing-additional-scripts-event branch from aa5e98a to fb01f17 Compare July 14, 2020 19:35
@juliusknorr
Copy link
Member Author

Fixed, rebased and squashed.

MorrisJobke
MorrisJobke approved these changes Jul 14, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Beside my little comment this looks good 👍

@MorrisJobke MorrisJobke force-pushed the enh/noid/sharing-additional-scripts-event branch from fb01f17 to 6c8ba1c Compare July 15, 2020 07:08
@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 15, 2020
@MorrisJobke
Copy link
Member

Ready for merge 🚀

skjnldsv
skjnldsv reviewed Jul 15, 2020
View reviewed changes
apps/files_sharing/lib/Controller/ShareController.php
@@ -478,7 +476,7 @@ public function showShare($path = ''): TemplateResponse {

// Load Viewer scripts
if (class_exists(LoadViewer::class)) {
$this->eventDispatcher->dispatch(LoadViewer::class, new LoadViewer());
$this->eventDispatcher->dispatchTyped(new LoadViewer());
Copy link
Member

Choose a reason for hiding this comment

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

We can do that? :O

Copy link
Member

Choose a reason for hiding this comment

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

Yep - that's the new way and makes things fully typed and thus allows the be a bit more safe.

Copy link
Member

Choose a reason for hiding this comment

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

Also you just register for the ::class and then the IDE finds properly all usages ;)

@MorrisJobke MorrisJobke force-pushed the enh/noid/sharing-additional-scripts-event branch from 6c8ba1c to 1aff8d7 Compare July 15, 2020 07:36
@MorrisJobke MorrisJobke force-pushed the enh/noid/sharing-additional-scripts-event branch from 1aff8d7 to 217a69e Compare July 15, 2020 11:55
rullzer
rullzer approved these changes Jul 15, 2020
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Looks good!

@MorrisJobke MorrisJobke merged commit 16d0ef9 into master Jul 15, 2020
@MorrisJobke MorrisJobke deleted the enh/noid/sharing-additional-scripts-event branch July 15, 2020 18:03
@ChristophWurst ChristophWurst mentioned this pull request Jul 15, 2020
Closed
This was referenced Jul 15, 2020
Merged
Merged
@MorrisJobke MorrisJobke removed the pending documentation This pull request needs an associated documentation update label Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

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 20
Development

Successfully merging this pull request may close these issues.
5 participants
@juliusknorr @MorrisJobke @rullzer @ChristophWurst @skjnldsv