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.

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

Properly emit Viewer event on files and files_sharing #19777

Merged
merged 3 commits into from
Mar 12, 2020

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Mar 4, 2020

Signed-off-by: John Molakvoæ (skjnldsv) skjnldsv@protonmail.com

@skjnldsv
Copy link
Member Author

skjnldsv commented Mar 4, 2020

/backport to stable18

@rullzer rullzer mentioned this pull request Mar 5, 2020
@rullzer rullzer modified the milestones: Nextcloud 18.0.2, Nextcloud 19 Mar 5, 2020
@skjnldsv skjnldsv requested a review from juliusknorr March 5, 2020 10:57
@skjnldsv skjnldsv force-pushed the fix/viewer-public branch from 134cbbf to 2d6a5b0 Compare March 6, 2020 13:57
@skjnldsv skjnldsv requested a review from kesselb March 6, 2020 13:57
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.

Looks good :)

apps/files_sharing/lib/Controller/ShareController.php Outdated Show resolved Hide resolved
apps/files_sharing/list.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv requested a review from kesselb March 6, 2020 14:45
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 9, 2020
@@ -68,8 +71,6 @@
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IManager as ShareManager;
use OCP\Template;
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
use Symfony\Component\EventDispatcher\GenericEvent;
Copy link
Member

Choose a reason for hiding this comment

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

Replacing Symfony\Component\EventDispatcher\GenericEvent with OCP\EventDispatcher\GenericEvent breaks the apps that listen to the OCA\Files_Sharing::loadAdditionalScripts and OCA\Files_Sharing::loadAdditionalScripts::publicShareAuth events, so this API change should be documented somewhere (although there are probably not a lot of apps using those events, but still ;-) ).

Copy link
Member Author

Choose a reason for hiding this comment

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

It does?
I don't understand why ? :)

Copy link
Member

Choose a reason for hiding this comment

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

It does if types are used in the event handler, as OCP\EventDispatcher\GenericEvent does not extend Symfony\Component\EventDispatcher\GenericEvent and thus can not replace it.

I have now seen that this pull request is meant to be backported to stable18. Please keep in mind that the event type change could go in for Nextcloud 19, but it should not be merged in stable18.

Copy link
Member Author

Choose a reason for hiding this comment

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

So reverting is the safest way?
Or should I just not backport the GenericEvent change?
What do you recommend here? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

So I can migrate to IEventDispatcher, but I should keep Symfony\Component\EventDispatcher\GenericEvent, right?

This comment was marked as outdated.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately you can not use IEventDispatcher either, because IEventDispatcher dispatches an OCP\EventDispatcher\Event, so it can not dispatch a Symfony\Component\EventDispatcher\GenericEvent. And as IEventDispatcher can not be used dispatchTyped can not be used either.

Similarly, dispatchTyped can not be used either in apps/files_sharing/list.php, because getEventDispatcher() returns a Symfony\Component\EventDispatcher\EventDispatcherInterface.

As EventDispatcherInterface is no longer replaced by IEventDispatcher the unit test fixes do not apply, so the commit was removed.

And finally, personally I would also drop the commit to use dispatchTyped, as it is used only once and while convenient it makes that call inconsistent with the rest of the LoadViewer dispatchers as well as with the other dispatchs in ViewCVontroller. But that is nitpicking, of course ;-) In any case, dispatchTyped was introduced in Nextcloud 18, so even if the commit is not dropped it should be safe to backport it to stable18.

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@danxuliu danxuliu force-pushed the fix/viewer-public branch from a1a6bc1 to 3cdadeb Compare March 11, 2020 08:47
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Mar 11, 2020
@skjnldsv
Copy link
Member Author

@danxuliu integration-remote-api fails

PHP Fatal error: Declaration of OC\EventDispatcher\SymfonyAdapter::dispatch($eventName, $event = NULL) must be compatible with Symfony\Contracts\EventDispatcher\EventDispatcherInterface::dispatch(object $event, ?string $eventName = NULL): object in /drone/src/lib/private/EventDispatcher/SymfonyAdapter.php on line 37
145

@danxuliu
Copy link
Member

PHP Fatal error: Declaration of OC\EventDispatcher\SymfonyAdapter::dispatch($eventName, $event = NULL) must be compatible with Symfony\Contracts\EventDispatcher\EventDispatcherInterface::dispatch(object $event, ?string $eventName = NULL): object in /drone/src/lib/private/EventDispatcher/SymfonyAdapter.php on line 37
145

Although it looks highly suspicious ;-) this is not caused by this pull request (it happens on master too), but by the Behat version bump in integration tests (which causes Symfony 5 instead of Symfony 4 to be installed by composer). I will send a fix for that (in another pull request, of course ;-) ).

@backportbot-nextcloud
Copy link

backport to stable18 in #19897

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: files feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants