Add logout prehooks by sharidas · Pull Request #29329 · owncloud/core

sharidas · 2017-10-23T13:00:56Z

This change will help to do actions
in logout prehooks. Any changes to
be made before logout action can be
done here.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Add logout prehooks

Related Issue

Motivation and Context

Adding logout prehooks

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

karakayasemi · 2017-10-23T13:06:58Z

optional, but we can consider following the naming convention of EventDispatcher.

karakayasemi · 2017-10-23T13:07:11Z

indentation

codecov · 2017-10-23T13:17:57Z

Codecov Report

Merging #29329 into master will increase coverage by <.01%.
The diff coverage is 77.77%.

@@             Coverage Diff              @@
##             master   #29329      +/-   ##
============================================
+ Coverage     60.21%   60.21%   +<.01%     
- Complexity    17182    17183       +1     
============================================
  Files          1030     1030              
  Lines         57271    57280       +9     
============================================
+ Hits          34485    34493       +8     
- Misses        22786    22787       +1

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Session.php	`62.61% <100%> (+1%)`	`118 <0> (+1)`	⬆️
lib/private/Server.php	`82.22% <33.33%> (-0.22%)`	`118 <0> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8d5f70...44b511c. Read the comment docs.

PVince81 · 2017-10-23T13:48:07Z

Don't abuse the session to pass around variables. There is an existing way to receive values from a symfony event, just use it. Here's an example: https://github.com/owncloud/guests/blob/master/controller/userscontroller.php#L130

sharidas · 2017-10-23T14:45:02Z

@PVince81 No more sessions. Data handled via Symfony events.

PVince81 · 2017-10-23T15:06:07Z

I think this should be called "cancel".

Also why not use booleans instead of strings containing booleans ?

sharidas · 2017-10-23T15:16:43Z

@PVince81 Updated the new patch by making 'cancel' and used booleans instead of string containing booleans. Thanks for the feedback.

PVince81 · 2017-10-23T15:21:42Z

Have you tested this ? You are not even firing the event with $eventDispatcher

Please look again at the example I posted and use the same approach

Its been fired from https://github.com/owncloud/impersonate/pull/68/files#diff-5ebb5b2aa16f3a5e11cb19da41fe2f7dR77.

Ah, that's a misunderstanding. Core must fire the event and apps must listen to it to react, not the other way around.

sharidas · 2017-10-23T17:45:49Z

@PVince81 Updated the change to trigger the event from core. And the impersonate app reacts to it. Thanks for the feedback.

sharidas · 2017-10-24T09:49:43Z

@PVince81 I have used naming convention for the event names. Let me know if this change is ok.

karakayasemi · 2017-10-24T11:33:55Z

"user.pre_logout_hook" is better i guess. According to the naming convention:
-Use only lowercase letters, numbers, dots (.) and underscores (_);
-Prefix names with a namespace followed by a dot (e.g. order., user.*);
-End names with a verb that indicates what action has been taken (e.g. order.placed).

PVince81 · 2017-10-25T08:04:22Z

 			});
+			$userSession->listen('\OC\User', 'preLogout', function () {
+				$event = new GenericEvent(null, []);
+				\OC::$server->getEventDispatcher()->dispatch('user.pre_logout_hook', $event);


hmmm, is this the official Symfony naming convention ? for all other Symfony events in OC we used a namespaced name, please look it up

https://symfony.com/doc/current/components/event_dispatcher.html#naming-conventions
Yes, this is the official Symfony naming convention, but we don't have to follow that of course. I checked quickly the other Symfony events in OC. Generally, we used a namespaced name, but 3rdparty libraries are following this convention, like:

core/apps/files_external/3rdparty/aws-sdk-php/Aws/Common/Model/MultipartUpload/AbstractTransfer.php

Line 33 in 5ce66c4

const BEFORE_UPLOAD = 'multipart_upload.before_upload';

or

core/apps/files_external/3rdparty/aws-sdk-php/Guzzle/Plugin/Backoff/BackoffPlugin.php

Line 19 in 5ce66c4

const RETRY_PARAM = 'plugins.backoff.retry_count';

All the events related to a class should be constant class property and we should use these constants when dispatching an event. IMHO it is cleaner.
By the way, core has some codes that follow the convention:

core/apps/files_sharing/lib/API/Remote.php

Line 116 in baf07a7

$dispatcher->dispatch('remoteshare.declined', $event);

or
https://github.com/owncloud/core/blob/master/lib/public/Http/HttpEvents.php#L32

For code consistency I am modifying the event name to use namespace.

@DeepDiver1975 any comment on this ? I assume we want to continue using the namespace format like we did for loadAdditionalScripts

AFAIK, we are still in the adaptation phase to EventDispatcher. IMHO, we can add event strings as constant property to the related class, we can track it under #29174 and follow the convention.

PVince81

please double check namespace format and add unit tests to make codecov happy.

PVince81 · 2017-10-25T13:31:50Z

+
+		$event = new GenericEvent(null, ['cancel' => false]);
+		$eventDispatcher = \OC::$server->getEventDispatcher();
+		$eventDispatcher->dispatch('\OC\User\Session::cancel_logout', $event);


No, the event should be \OC\User\Session::pre_logout. We are informing every listener that we are about to logout, not about the cancel.

PVince81 · 2017-10-25T13:32:04Z

 			});
+			$userSession->listen('\OC\User', 'preLogout', function () {
+				$event = new GenericEvent(null, []);
+				\OC::$server->getEventDispatcher()->dispatch('\OC\User\Session::pre_logouthook', $event);


"pre_logout", remove the "hook" from the name

sharidas · 2017-10-25T14:27:39Z

@PVince81 Hope the code looks better.

PVince81

👍 thanks, please backport

sharidas · 2017-10-26T02:54:38Z

Backport PR: #29352

This change will help to do actions in logout prehooks. Any changes to be made before logout action can be done here. Signed-off-by: Sujith H <sharidasan@owncloud.com>

lock · 2019-08-02T05:01:56Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

sharidas self-assigned this Oct 23, 2017

sharidas requested a review from PVince81 October 23, 2017 13:00

sharidas added the 2 - Developing label Oct 23, 2017

sharidas added this to the development milestone Oct 23, 2017

karakayasemi reviewed Oct 23, 2017

View reviewed changes

sharidas force-pushed the logout-prehooks branch 2 times, most recently from 31ac4bf to 79c47cb Compare October 23, 2017 13:17

sharidas force-pushed the logout-prehooks branch from 79c47cb to f75b7ef Compare October 23, 2017 13:37

PVince81 reviewed Oct 23, 2017

View reviewed changes

sharidas force-pushed the logout-prehooks branch from f75b7ef to 1037226 Compare October 23, 2017 14:44

PVince81 reviewed Oct 23, 2017

View reviewed changes

sharidas force-pushed the logout-prehooks branch 2 times, most recently from 685b513 to 4fb0892 Compare October 23, 2017 15:14

PVince81 reviewed Oct 23, 2017

View reviewed changes

sharidas mentioned this pull request Oct 23, 2017

Removing logout logic from the js owncloud/impersonate#68

Merged

sharidas force-pushed the logout-prehooks branch 2 times, most recently from 1e536bf to 32039d5 Compare October 23, 2017 17:42

sharidas force-pushed the logout-prehooks branch 2 times, most recently from 42b213c to 370ae0f Compare October 24, 2017 09:47

sharidas changed the title ~~[WIP] Add logout prehooks~~ Add logout prehooks Oct 24, 2017

karakayasemi reviewed Oct 24, 2017

View reviewed changes

sharidas force-pushed the logout-prehooks branch from 370ae0f to 8988368 Compare October 24, 2017 11:44

sharidas added 3 - To Review and removed 2 - Developing labels Oct 24, 2017

PVince81 reviewed Oct 25, 2017

View reviewed changes

sharidas force-pushed the logout-prehooks branch 2 times, most recently from 13c4c61 to 3af8477 Compare October 25, 2017 10:27

PVince81 reviewed Oct 25, 2017

View reviewed changes

sharidas force-pushed the logout-prehooks branch from 3af8477 to eb1384d Compare October 25, 2017 14:24

PVince81 approved these changes Oct 25, 2017

View reviewed changes

PVince81 added 4 - To release backport-request and removed 3 - To Review labels Oct 25, 2017

sharidas force-pushed the logout-prehooks branch 2 times, most recently from d4ba906 to d859408 Compare October 26, 2017 03:18

Add logout prehooks

44b511c

This change will help to do actions in logout prehooks. Any changes to be made before logout action can be done here. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the logout-prehooks branch from d859408 to 44b511c Compare October 26, 2017 06:02

sharidas merged commit c12854c into master Oct 26, 2017

sharidas deleted the logout-prehooks branch October 26, 2017 08:44

PVince81 removed the backport-request label Nov 3, 2017

sharidas mentioned this pull request Nov 28, 2017

[stable10] Add logout prehooks #29352

Merged

9 tasks

ownclouders added the status/STALE label Dec 20, 2017

lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Uh oh!

Conversation

sharidas commented Oct 23, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Oct 23, 2017

Uh oh!

sharidas commented Oct 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karakayasemi Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sharidas commented Oct 25, 2017

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

sharidas commented Oct 26, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Oct 23, 2017 •

edited

Loading

karakayasemi Oct 25, 2017 •

edited

Loading