Skip to content

Run rector on lib/private #56496

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
Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Run rector on lib/private #56496

wants to merge 8 commits into from
+3,252 −4,771

Conversation

@CarlSchwan
Copy link
Member

@CarlSchwan CarlSchwan commented Nov 17, 2025 •
edited
Loading

  • Resolves: #

Summary

Seems to work fine locally and the CI is also happy

I ended up doing some manual changes to lib/private/Group/Group.php, to lib/private/Server.php and to lib/private/AppFramework/DependencyInjection/DIContainer.php to make it work

TODO

  • ...

Checklist

@CarlSchwan CarlSchwan added this to the Nextcloud 33 milestone Nov 17, 2025
@CarlSchwan CarlSchwan self-assigned this Nov 17, 2025
@CarlSchwan CarlSchwan added the 2. developing Work in progress label Nov 17, 2025
@CarlSchwan CarlSchwan marked this pull request as ready for review November 18, 2025 11:49
@CarlSchwan CarlSchwan requested review from provokateurin, salmart-dev and yemkareems and removed request for a team November 18, 2025 11:49
ChristophWurst
ChristophWurst reviewed Nov 18, 2025
View reviewed changes
@provokateurin
Copy link
Member

provokateurin commented Nov 18, 2025

We were always a bit hesitant to do this, but I guess we just have to do it at some point. I'm fine with doing it now, if there is anything wrong we hopefully have enough time until 33.
GH isn't able to let me see the entire second commit, so I can only really yolo this one.

@CarlSchwan
fix(IAppContainer): Expose more correct type
d10eb02 
OC\ServerContainer is not inherinting from IServerContainer so expose
the most near type.

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Copy link
Member Author

CarlSchwan commented Nov 18, 2025

We were always a bit hesitant to do this, but I guess we just have to do it at some point. I'm fine with doing it now, if there is anything wrong we hopefully have enough time until 33. GH isn't able to let me see the entire second commit, so I can only really yolo this one.

We do need to bit the bullet at some point :)

miaulalala
miaulalala requested changes Nov 18, 2025
View reviewed changes
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First batch

miaulalala
miaulalala requested changes Nov 18, 2025
View reviewed changes
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First batch

@CarlSchwan
refactor: Apply comments
81a301f 
Signed-off-by: Carl Schwan <[email protected]>
miaulalala
miaulalala requested changes Nov 19, 2025
View reviewed changes
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Batch 2 🥇

miaulalala
miaulalala requested changes Nov 19, 2025
View reviewed changes
Copy link
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Batch 2 🥇

@CarlSchwan
Copy link
Member Author

CarlSchwan commented Nov 20, 2025 •
edited
Loading

Batch 2 🥇

Thanks for the careful review :) Applied almost everything, aside of a few things that were breaking some other parts

salmart-dev
salmart-dev requested changes Nov 21, 2025
View reviewed changes
Copy link
Contributor

@salmart-dev salmart-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the small issues in the comments I noticed many changes in constructor parameter names. Those would be breaking changes since PHP has named parameters support, but I assume that we don't use that internally. I think all classes I saw with this kind of change were all internal.

lib/private/Authentication/Login/TwoFactorCommand.php
@@ -68,7 +58,6 @@ public function process(LoginData $loginData): LoginResult {
}

return LoginResult::success(
$loginData,
Copy link
Contributor

@salmart-dev salmart-dev Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why is this argument being removed? I see the function still has two parameters

Copy link
Member Author

@CarlSchwan CarlSchwan Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the loginData parameter was unused. I also removed it from the function

@CarlSchwan CarlSchwan added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst left review comments

@salmart-dev salmart-dev salmart-dev requested changes

@miaulalala miaulalala miaulalala requested changes

@SebastianKrupinski SebastianKrupinski Awaiting requested review from SebastianKrupinski SebastianKrupinski is a code owner

@tcitworld tcitworld Awaiting requested review from tcitworld tcitworld is a code owner

@nickvergessen nickvergessen Awaiting requested review from nickvergessen nickvergessen is a code owner

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner

@yemkareems yemkareems Awaiting requested review from yemkareems yemkareems is a code owner automatically assigned from nextcloud/server-backend

@provokateurin provokateurin Awaiting requested review from provokateurin provokateurin is a code owner automatically assigned from nextcloud/server-backend

Requested changes must be addressed to merge this pull request.

Assignees

@CarlSchwan CarlSchwan

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 33

Development

Successfully merging this pull request may close these issues.

6 participants

@CarlSchwan @provokateurin @ChristophWurst @salmart-dev @miaulalala