Skip to content

Add LoadAdditionalScriptsEvent for files_sharing - #21815

Merged
MorrisJobke merged 1 commit into
masterfrom
enh/noid/sharing-additional-scripts-event
Jul 15, 2020
Merged

Add LoadAdditionalScriptsEvent for files_sharing#21815
MorrisJobke merged 1 commit into
masterfrom
enh/noid/sharing-additional-scripts-event

Conversation

@juliusknorr

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

Copy link
Copy Markdown
Member

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.

@juliusknorrjuliusknorr added enhancement 3. to review Waiting for reviews technical debt 🧱 🤔🚀 labels Jul 13, 2020
@juliusknorrjuliusknorr added this to the Nextcloud 20 milestone Jul 13, 2020
@juliusknorrjuliusknorr mentioned this pull request Jul 13, 2020
Merged
MorrisJobke
MorrisJobke reviewed Jul 13, 2020
View reviewed changes
Comment threadapps/files_sharing/lib/Listener/LegacyLoadAdditionalScriptsAdapter.php Outdated
MorrisJobke
MorrisJobke reviewed Jul 13, 2020
View reviewed changes
Comment threadapps/files_sharing/lib/Listener/LegacyLoadAdditionalScriptsAdapter.php Outdated
MorrisJobke
MorrisJobke reviewed Jul 13, 2020
View reviewed changes
Comment threadapps/files_sharing/lib/Controller/ShareController.php Outdated
@MorrisJobke

MorrisJobke commented Jul 13, 2020

Copy link
Copy Markdown
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

MorrisJobke commented Jul 13, 2020

Copy link
Copy Markdown
Member

#14552 (comment)

MorrisJobke
MorrisJobke approved these changes Jul 13, 2020
View reviewed changes

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

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
MorrisJobkeforce-pushed the enh/noid/sharing-additional-scripts-event branch from 16ce985 to b329c95CompareJuly 13, 2020 20:24
@MorrisJobke

MorrisJobke commented Jul 13, 2020

Copy link
Copy Markdown
Member

Rebased and autosquashed 🚀

@MorrisJobke
MorrisJobkeforce-pushed the enh/noid/sharing-additional-scripts-event branch from b329c95 to aa5e98aCompareJuly 13, 2020 20:30
@MorrisJobke

MorrisJobke commented Jul 13, 2020

Copy link
Copy Markdown
Member

And I fixed the unit tests 🙈

This was referenced Jul 13, 2020
Merged
Merged
@ChristophWurstChristophWurst 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

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

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.

Comment threadapps/files_sharing/lib/Event/LoadAdditionalScriptsEvent.php Outdated
@MorrisJobkeMorrisJobke mentioned this pull request Jul 14, 2020
Merged
2 tasks
@juliusknorr
juliusknorrforce-pushed the enh/noid/sharing-additional-scripts-event branch from aa5e98a to fb01f17CompareJuly 14, 2020 19:35
@juliusknorr

juliusknorr commented Jul 14, 2020

Copy link
Copy Markdown
MemberAuthor

Fixed, rebased and squashed.

MorrisJobke
MorrisJobke reviewed Jul 14, 2020
View reviewed changes
Comment threadapps/files_sharing/lib/AppInfo/Application.php Outdated
MorrisJobke
MorrisJobke approved these changes Jul 14, 2020
View reviewed changes

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

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
MorrisJobkeforce-pushed the enh/noid/sharing-additional-scripts-event branch from fb01f17 to 6c8ba1cCompareJuly 15, 2020 07:08
@MorrisJobkeMorrisJobke 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

MorrisJobke commented Jul 15, 2020

Copy link
Copy Markdown
Member

Ready for merge 🚀

skjnldsv
skjnldsv reviewed Jul 15, 2020
View reviewed changes
Comment threadapps/files_sharing/lib/Controller/ShareController.php
// Load Viewer scripts
if (class_exists(LoadViewer::class)) {
$this->eventDispatcher->dispatch(LoadViewer::class, new LoadViewer());
$this->eventDispatcher->dispatchTyped(new LoadViewer());

@skjnldsvskjnldsvJul 15, 2020

Copy link
Copy Markdown
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

@MorrisJobkeMorrisJobkeJul 15, 2020

Copy link
Copy Markdown
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.

@MorrisJobkeMorrisJobkeJul 15, 2020

Copy link
Copy Markdown
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 ;)

skjnldsv
skjnldsv reviewed Jul 15, 2020
View reviewed changes
Comment threadapps/files_sharing/lib/Event/BeforeTemplateRenderedEvent.php Outdated
@MorrisJobke
MorrisJobkeforce-pushed the enh/noid/sharing-additional-scripts-event branch from 6c8ba1c to 1aff8d7CompareJuly 15, 2020 07:36
@juliusknorr@MorrisJobke
Add BeforeTemplateRenderedEvent for files_sharing
217a69e
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@MorrisJobke
MorrisJobkeforce-pushed the enh/noid/sharing-additional-scripts-event branch from 1aff8d7 to 217a69eCompareJuly 15, 2020 11:55
rullzer
rullzer approved these changes Jul 15, 2020
View reviewed changes

@rullzerrullzer left a comment

Copy link
Copy Markdown
Member

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 masterJul 15, 2020
@MorrisJobke
MorrisJobke deleted the enh/noid/sharing-additional-scripts-event branch July 15, 2020 18:03
@ChristophWurstChristophWurst mentioned this pull request Jul 15, 2020
Closed
This was referenced Jul 15, 2020
Merged
Merged
@MorrisJobkeMorrisJobke removed the pending documentation This pull request needs an associated documentation update label Jul 15, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobkeMorrisJobkeMorrisJobke approved these changes
@skjnldsvskjnldsvskjnldsv approved these changes
@rullzerrullzerrullzer approved these changes
@ChristophWurstChristophWurstChristophWurst approved these changes
@nickvergessennickvergessenAwaiting requested review from nickvergessen

Assignees

No one assigned

Labels

4. to releaseReady to be released and/or waiting for tests to finishenhancementtechnical debt🧱 🤔🚀

Projects

None yet

Milestone

Nextcloud 20

Development

Successfully merging this pull request may close these issues.

5 participants

@juliusknorr@MorrisJobke@rullzer@ChristophWurst@skjnldsv