AllConfig setUserValue opt #1734
Conversation
|
@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @schiessle, @MorrisJobke and @nickvergessen to be potential reviewers. |
lib/private/AllConfig.php
Outdated
|
|
||
| if (isset($this->userCache[$userId])) { | ||
| if (isset($this->userCache[$userId][$appName])) { | ||
| if (isset($this->userCache[$userId][$appName][$key])) { |
There was a problem hiding this comment.
Those 3 if statements could be one ;) best would be each isset on a new line ;)
There was a problem hiding this comment.
Ah yes fair enough. I was actually in the process of adding tests. Which is kind of impossible since we can't do proper DI here at the moment :S
There was a problem hiding this comment.
Also if (isset($this->userCache[$userId][$appName][$key])) { is sufficient, php checks parents first and doesnt fail
Signed-off-by: Roeland Jago Douma <[email protected]>
Signed-off-by: Roeland Jago Douma <[email protected]>
Current coverage is 57.05% (diff: 85.71%)@@ master #1734 diff @@
==========================================
Files 1064 1064
Lines 60396 60981 +585
Methods 6818 6937 +119
Messages 0 0
Branches 0 0
==========================================
+ Hits 34235 34791 +556
- Misses 26161 26190 +29
Partials 0 0
|
|
@nickvergessen @MorrisJobke fixed :) please review. |
|
Somehow autocomplete for LDAP users doesn't work anymore for all users in my local instance with this :( |
|
@rullzer what about a unit test? |
|
I can't properly unit test because there is no proper DI |
|
I'll look into the ldap issue |
|
@MorrisJobke mmm ldap still works here. I don't get it. |
|
We could split out the userconfig from AllConfig. So we can do proper DI there. then AllConfig is just the external wrapper. Then we could do fancy unit tests |
|
Tested and works 👍 Sorry - I mistyped the user and then the autocomplete didn't worked 😉 |
|
LGTM |
I noticed that when we do a SetUser value we first do and insert and if that fails we do an update. This for example happens everytime we refresh the ldap cache for a user at least 5 times.
Since often we have the user config fetched anyways we should take advantage of it. And only update if needed and if needed just update.
CC: @LukasReschke @nickvergessen @MorrisJobke