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

Include the share in the Files_Sharing::loadAdditionalScripts event #17575

Merged
merged 1 commit into from
Oct 19, 2019
Merged

Include the share in the Files_Sharing::loadAdditionalScripts event #17575

merged 1 commit into from
Oct 19, 2019

Conversation

danxuliu
Copy link
Member

This provides a better context for apps using the event, for example to load one script or another depending on whether the share is a file or a folder.

This should be backported to Nextcloud 17, as it is needed by Talk to fix some issues in the public share page for folder shares.

@danxuliu danxuliu added this to the Nextcloud 18 milestone Oct 17, 2019
@danxuliu
Copy link
Member Author

/backport to stable17

@danxuliu
Copy link
Member Author

Wait, do not review yet 🤦‍♂️

@danxuliu danxuliu added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 17, 2019
@danxuliu
Copy link
Member Author

Done.

The tests passed because only the first parameter was being checked; I have extended them to also test the new parameter.

I have also changed the parameter from $this->share to $share; in the real world it should not matter, as $this->share is automatically set to $this->shareManager->getShareByToken($this->getToken()) in isValidToken() before the controller is executed, but $share is used throughout showShare() instead of $this->share, so keep consistency with that.

@danxuliu danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Oct 17, 2019
rullzer
rullzer approved these changes Oct 17, 2019
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.

Those should be converted to the new events 😉

But sure.

@danxuliu
Include the share in the Files_Sharing::loadAdditionalScripts event
5c7c308 
This provides a better context for apps using the event, for example to
load one script or another depending on whether the share is a file or a
folder.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the include-the-share-in-the-files_sharing-loadadditionalscripts-event branch from f6c38e5 to 5c7c308 Compare October 17, 2019 16:24
@danxuliu
Copy link
Member Author

Squashed.

@rullzer rullzer merged commit b425b34 into master Oct 19, 2019
@rullzer rullzer deleted the include-the-share-in-the-files_sharing-loadadditionalscripts-event branch October 19, 2019 09:13
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Oct 19, 2019
Merged
@backportbot-nextcloud
Copy link

backport to stable17 in #17597

@danxuliu danxuliu mentioned this pull request Oct 21, 2019
Merged
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

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

Assignees
No one assigned
Labels
3. to review Waiting for reviews feature: sharing
Projects
None yet
Milestone
Nextcloud 18.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@danxuliu @rullzer @juliusknorr