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

Run rector on lib/private #56496

CarlSchwan wants to merge 9 commits into from
+3,543 −5,812

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 force-pushed the carl/rector branch 2 times, most recently from fe8cd31 to 22cc35c Compare November 18, 2025 10:20
@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
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 CarlSchwan force-pushed the carl/rector branch 2 times, most recently from ce62cda to 95fbaeb Compare November 19, 2025 10:50
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 🥇

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.

@CarlSchwan CarlSchwan added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 24, 2025
miaulalala
miaulalala requested changes Nov 24, 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.

didn't check the rest of the test files, I assume they could also be strongly typed.

Last batch 🥳

@CarlSchwan CarlSchwan force-pushed the carl/rector branch 13 times, most recently from 8e9a599 to bca103f Compare December 2, 2025 21:34
@CarlSchwan
chore(rector): Run rector on apps, core and tests directory
623fcd2 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
refactor: Run rector on lib/private
ff47223 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
fix(IAppContainer): Expose more correct type
574dcf9 
OC\ServerContainer is not inherinting from IServerContainer so expose
the most near type.

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
refactor: Apply comments
6acc68c 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
fix(ExpressionBuilderTest): Fix test and improve typing
60d9102 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
fix(ShareAPIControllerTest): Fix return types of DateTimeZone
d7a9dd8 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
refactor: Apply second batch of comments
54c7bb0 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
refactor: Update repairs jobs
58d4405 
Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
refactor: Add more typing
cc09afe 
- repairs job
- database
- redis

And remove Helpertest which was unused outside of some tests.

Signed-off-by: Carl Schwan <[email protected]>
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

@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

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

@salmart-dev salmart-dev Awaiting requested review from salmart-dev

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