Skip to content
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.

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

refactor(apps): Use constructor property promotion when possible #48790

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

Summary

Done with rector. Maybe this PR is a bit too big, I can make separate PRs for individual apps if needed.

Checklist

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Checked apps/dav and some other random samples. Can't see any issues

@nickvergessen
Copy link
Member

In terms of backporting I'm a fan of not doing it, unless it's touched anyway 🙈
But of course clean code is also fine and resolving the conflict should be doable

@provokateurin
Copy link
Member Author

I would not backport this, I tried to apply it locally and there are too many conflicts that are not worth spending the time on.

@nickvergessen
Copy link
Member

  • Psalm has some comments
  • the one thing I wonder is in some cases we type hint the member to an actual implementation, while the injection is done with the public interface, because not all methods are in the public interface. Of course I don't know a place out of my head, but do you know how it handles that?

@provokateurin
Copy link
Member Author

As far as I can tell it just removes the property and adds the promotion in the constructor. That means if the injection uses the interface we will also use that internally. Judging from the failed tests and the psalm results this doesn't seem to cause problems with every used method already being in the interface (which is really good 🎉)

@provokateurin provokateurin force-pushed the refactor/apps/constructor-property-promotion branch from 8b512ea to d361ac9 Compare October 18, 2024 10:48
@provokateurin
Copy link
Member Author

Only problem I found was where the property was called uid which was then renamed in the constructor as well, so it was no longer userId and could not be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants