Fix EZP-21952: user anonymous cannot be redefined #642
Conversation
|
Regarding loading default anonymous user for the repo - this is done when calling Ping @andrerom @lolautruche @docb22 |
|
@pspanja username being unique, "anonymous" can only be one, so it does not seem to solve / help the problem of having different anonymous users (per siteaccess). As for hardcoding username instead of id: it never was done before, and I'm not sure if it adds much value. 1) People could use "anon" or "anonyme" username, and 2) basically the only case I could see it being of use is if you delete by accident mr. 10 and then recreate it with same name but different id |
| ->end() | ||
| ->scalarNode( 'anonymous_user_id' ) | ||
| ->cannotBeEmpty() | ||
| ->example( '10' ) |
There was a problem hiding this comment.
Please use integerNode() instead of scalarNode(). And example must be a integer as well.
There was a problem hiding this comment.
Actually our ids are mixed (int/string), so we need to allow for that. Will update the example.
|
I'm really not in favor of deprecating |
|
@gggeek There is also the benefit of removing storage awareness. But I actually agree, this is why I would rather remove this behaviour and require that user is explicitly set. |
|
@lolautruche it turns out it is only a convenience method. To have this really usable, we would need to configure repository with anon. user id and then have this method falling back to loadUser( $anonUserId ). It is simpler to use that in the first place and keep unnecessary stuff out of repo. |
| @@ -79,7 +79,6 @@ public function __construct( RepositoryInterface $repository, Handler $userHandl | |||
| $this->userHandler = $userHandler; | |||
| // Union makes sure default settings are ignored if provided in argument | |||
| $this->settings = $settings + array( | |||
| 'anonymousUserID' => 10, | |||
There was a problem hiding this comment.
Should probably still be here as fallback given it is used further down in loadAnonymousUser still.
There was a problem hiding this comment.
@andrerom This setting is moved up to the Repository, as it is now required by getCurrentUser method.
Any opinion on changing the behaviour of that method regarding loading default anonymous user, as suggested in my comment above?
There was a problem hiding this comment.
Which suggestions specifically, could you rephrase the suggestion?
There was a problem hiding this comment.
We now have default current user (anonymous), used if current user is not explicitly set.
Its ID is hardcoded, which is storage specific. So we should fix this somehow, my suggestions:
- load default current user by user name, "anonymous" in this case - would work for other storages as well if such user was in the DB
- remove it completely and require that current user is explicitly loaded and set
There was a problem hiding this comment.
@andrerom since this is a customer issue I guess this change should be deferred in any case.
Are you ok with the setting being moved up to the Repository?
|
Beside comment, +1 |
|
All todos are resolved now, review ping @andrerom @lolautruche @docb22 |
|
+1, even if it will collide a bit with #656 😃 |
|
@lolautruche Changed as requested, please check. |
|
+1, thanks 😃 |
Fix EZP-21952: user anonymous cannot be redefined
This PR resolves issue https://jira.ez.no/browse/EZP-21952
This adds possibility to configure custom anonymous users.
Since setting anonymous user to the repository can be done in layers above PAPI,
UserService::loadAnonymousUser()is deprecated in favour of usingUserService::loadUser( $anonymousUserId ).Regarding other settings ATM hardcoded in UserService:
TODOs
update configuration in community repoloading default anonymous user by id: replace with loading by user name / remove this behaviour completely