-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
lib/private/User: always pass old value to 'triggerChange' in User class, do not change user properties if value has not changed #14566
Conversation
@@ -365,7 +365,8 @@ public function setEnabled(bool $enabled = true) { | |||
$oldStatus = $this->isEnabled(); | |||
$this->enabled = $enabled; | |||
if ($oldStatus !== $this->enabled) { | |||
$this->triggerChange('enabled', $enabled); | |||
// TODO: First change the value, then trigger the event as done for all other properties. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with moving it one line to the bottom to be after the change was made. 👍 Could you make this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this will change behavior: Everyone currently rejecting an changeUser
event where enabled
is modified by throwing an exception would need to revert the change to enabled
(as enabled
will then be updated even though the exception is thrown).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was never meant for this and doing this is also not the way to handle such stuff. It's highly recommended to not manipulate data via this mechanism. This is also documented and communicated like this.
Also I couldn't find any code that does it like that. Do you know any app that does this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also those events are pure one way only events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also documented and communicated like this.
Oh, where?
Do you know any app that does this?
Our (Struktur-proprietary) app needs to reject modifications to a user under certain conditions, throwing an exception from a changeUser
-hook callback works great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also documented and communicated like this.
Oh, where?
I thought it was - but https://docs.nextcloud.com/server/stable/developer_manual/app/hooks.html doesn't has it. Maybe it was only on Github and never made it into the docs 😢.
Do you know any app that does this?
Our (Struktur-proprietary) app needs to reject modifications to a user under certain conditions, throwing an exception from a
changeUser
-hook callback works great.
If at all then there needs to be a pre
event to differentiate between the use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, see #14565.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code in general looks good 👍
$result = $this->backend->setDisplayName($this->uid, $displayName); | ||
if ($result) { | ||
$this->displayName = $displayName; | ||
$this->triggerChange('displayName', $displayName); | ||
$this->triggerChange('displayName', $displayName, $oldDisplayName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the other places where this hook is triggered:
lib/private/User/User.php
(remove or adapt?)
Also there is
apps/user_ldap/lib/User/User.php
because it is not called externally, but when changes in LDAP were discovered. This cannot be removed, should be adopted. It should not intervene with User:setDisplayName.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
fcfa7de
to
1f00338
Compare
@rullzer @nickvergessen @blizzz Mind to review this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure... 🐘
@leonklingele-work @leonklingele Mind to add the sign-off messages like described in https://github.com/nextcloud/server/pull/14566/checks?check_run_id=75870774 ? Then this one can go in. While doing this please also rebase on latest master - there we fixed the failing tests. |
Some tests are failing and need adjustments. Others (LDAP integration) as well, but this does not seem to be due to this changes → a rebase would be nice. |
Rebased version to check CI: #14788 |
@leonklingele-work Mind to check out the failing unit tests? |
…er::triggerChange
…en issue event A new event, 'preChangeUser', should be used for this instead. See nextcloud#14565.
1b128be
to
fe9f121
Compare
Still failing tests: Status of 17446: failureENABLE_REDIS=true, TESTS=db-codecovShow full log
DB=sqlite, ENABLE_REDIS=true, PHP=7.1Show full log
DB=sqlite, ENABLE_REDIS=false, PHP=7.2Show full log
DB=sqlite, ENABLE_REDIS=false, PHP=7.3Show full log
DB=mysql, ENABLE_REDIS=true, PHP=7.1Show full log
DB=mysql, ENABLE_REDIS=false, PHP=7.2Show full log
DB=mysql, ENABLE_REDIS=false, PHP=7.3Show full log
DB=mysql5.6, ENABLE_REDIS=true, PHP=7.1Show full log
DB=mysql5.5, ENABLE_REDIS=true, PHP=7.1Show full log
DB=postgres, ENABLE_REDIS=true, PHP=7.1, POSTGRES=9Show full log
DB=postgres, ENABLE_REDIS=true, PHP=7.1, POSTGRES=10Show full log
DB=mysqlmb4, ENABLE_REDIS=true, PHP=7.1Show full log
DB=mysqlmb4, ENABLE_REDIS=false, PHP=7.2Show full log
DB=mysqlmb4, ENABLE_REDIS=false, PHP=7.3Show full log
|
For me the tests also fail on master:
|
Works here: https://drone.nextcloud.com/nextcloud/server/17450 🤔 |
But they fail locally for me as well. Let me bisect. |
Seems to be a local issue. I tried all of them and all fail:
I guess we found a race condition here - could that be? One that never showed up on CI, but on local runs only? |
Let's get this in an check if the new drone architecture also triggers this. |
Opened in a new PR to debug failing CI: #14967 |
Partially added via #15052 to properly debug the CI failure in smaller steps. |
No description provided.