Skip to content

Inject public IGroupManager instead of private GroupManager #8797

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.

Sign up for GitHub

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

Jump to bottom
Merged
MorrisJobke merged 1 commit into from
Mar 13, 2018
Merged

Inject public IGroupManager instead of private GroupManager #8797

MorrisJobke merged 1 commit into from
Mar 13, 2018

Conversation

@danxuliu
Copy link
Member

@danxuliu danxuliu commented Mar 13, 2018 •
edited
Loading

This pull request fixes a regression introduced in #8743

The public IGroupManager service returned by the dependency injection system is automatically initialized with an OC\Group\Database backend. However, no backend is automatically set in private GroupManager instances. Therefore, a private GroupManager instance does not work as expected when initialized through the dependency injection system.

For example, an admin can no longer change the password of another user, and creating another user in the admin group does not fully work; although the user is created an error is returned anyway by the server.

Due to that this pull request reverts the changes from #8743 to replace the public "IGroupManager" with a private "GroupManager" instance. It seems that using again the public "IGroupManager" does not cause any problem regarding the strict types, so maybe something else was changed in the meantime that made that previous change no longer needed.

@danxuliu danxuliu added this to the Nextcloud 14 milestone Mar 13, 2018
@danxuliu danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 13, 2018
@danxuliu danxuliu added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 13, 2018
@danxuliu
Copy link
Member Author

danxuliu commented Mar 13, 2018

🤦 Of course it does cause problems with the strict types...

I have just realized that getSubAdmin() is not part of IGroupManager API. I guess that the reason that phan does not complain in Drone yet is simply because it will complain once #8375 is merged.

Sorry, long night :-)

Unfortunately I do not know how to solve this due to the lack of OC\Group\Database backend in the private GroupManager; an obvious way would be to inject the backend too in the constructor and explicitly add it to GroupManager... but I guess that there are way better approaches that my sleepiness does not allow me to see :-P

@rullzer
Copy link
Member

rullzer commented Mar 13, 2018

@danxuliu as a quick fix just remove the strict type declaration for now.

@danxuliu
Inject public IGroupManager instead of private GroupManager
044d5a8 
The public "IGroupManager" service returned by the dependency injection
system is automatically initialized with an "OC\Group\Database" backend.
However, no backend is automatically set in private "GroupManager"
instances. Therefore, a private "GroupManager" instance does not work as
expected when initialized through the dependency injection system.

Due to that this commit reverts a previous change in which the public
"IGroupManager" was replaced by a private "GroupManager" instance. That
change was needed when strict types were set, as "getSubAdmin()" is not
part of "IGroupManager" API, so the type had to be changed to
"GroupManager". Until a better solution is found strict types are
disabled again to be able to inject "IGroupManager" and also use
"getSubAdmin()".

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the inject-public-igroupmanager-instead-of-private-groupmanager branch from 7e4ca46 to 044d5a8 Compare March 13, 2018 08:53
@codecov
Copy link

codecov bot commented Mar 13, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@7488218). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #8797   +/-   ##
=========================================
  Coverage          ?   51.87%           
  Complexity        ?    25357           
=========================================
  Files             ?     1607           
  Lines             ?    95100           
  Branches          ?     1377           
=========================================
  Hits              ?    49336           
  Misses            ?    45764           
  Partials          ?        0
Impacted Files Coverage Δ Complexity Δ
settings/Controller/ChangePasswordController.php 16.36% <0%> (ø) 26 <0> (?)
settings/Controller/UsersController.php 68.83% <0%> (ø) 121 <0> (?)

@danxuliu
Copy link
Member Author

danxuliu commented Mar 13, 2018

@rullzer Done.

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 13, 2018
@MorrisJobke
Copy link
Member

MorrisJobke commented Mar 13, 2018

CI for login tests is green and solution was proposed by @rullzer anyways -> merging.

@MorrisJobke MorrisJobke merged commit a827160 into master Mar 13, 2018
@MorrisJobke MorrisJobke deleted the inject-public-igroupmanager-instead-of-private-groupmanager branch March 13, 2018 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug feature: settings regression

Projects

None yet

Milestone

Nextcloud 14

Development

Successfully merging this pull request may close these issues.

4 participants

@danxuliu @rullzer @MorrisJobke