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.

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

Use PSR container interface and deprecate our own abstraction #21809

Merged
merged 1 commit into from
Jul 16, 2020
Merged

Use PSR container interface and deprecate our own abstraction #21809

merged 1 commit into from
Jul 16, 2020

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Jul 13, 2020 •
edited
Loading

For https://help.nextcloud.com/t/do-we-still-need-ocp-icontainer/86139

  • Add the PSR-11 container interface as a replacement for IContainer
  • Deprecate our IContainer and friends
  • Make the new interface base of our abstraction to ease the migration
  • Use composition instead of inheritance for SimpleContainer -> Pimple as that makes the abstraction leaner and will help with the migration later on.

Docs are at nextcloud/documentation#2202

@ChristophWurst ChristophWurst added the 2. developing Work in progress label Jul 13, 2020
@ChristophWurst ChristophWurst added this to the Nextcloud 20 milestone Jul 13, 2020
@ChristophWurst ChristophWurst self-assigned this Jul 13, 2020
rullzer
rullzer requested changes Jul 13, 2020
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Maybe I'm not 100% right with the PSR... but it seems has and get have clear defined meanings...

lib/private/AppFramework/Utility/SimpleContainer.php Outdated Show resolved Hide resolved
lib/private/AppFramework/Utility/SimpleContainer.php Outdated Show resolved Hide resolved
@ChristophWurst ChristophWurst mentioned this pull request Jul 16, 2020
Merged
@ChristophWurst ChristophWurst marked this pull request as ready for review July 16, 2020 07:38
@ChristophWurst
Copy link
Member Author

FYI one of my attempts was to get rid of Pimple and replace it with Symfony. But that container is not easy to use as it kind of expects to be first notified about all the known services, then compiled and finally used. But we have a more dynamic approach and allow registration later on. So I switched to the League container and that felt a lot more natural to our DI usage. However, the changes are major and we (@rullzer and I) think it would be a bad idea to cram this in so late in the 20 cycle. Thus this will be done for 21.

The important part is the new PSR API. That is done with this PR. So the migration can start.

rullzer
rullzer approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

🚀

This was referenced Jul 16, 2020
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@ChristophWurst
Copy link
Member Author

ChristophWurst commented Jul 16, 2020 •
edited
Loading

I don't see any files anymore 🙈 fixed

Let's see if CI can find the faults.

@ChristophWurst ChristophWurst added pending documentation This pull request needs an associated documentation update 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jul 16, 2020
@ChristophWurst ChristophWurst mentioned this pull request Jul 16, 2020
Merged
MorrisJobke
MorrisJobke approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Fine by me 👍 Also changing to a PSR standard is the way to go

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 16, 2020
@ChristophWurst ChristophWurst merged commit 33aeef2 into master Jul 16, 2020
@ChristophWurst ChristophWurst deleted the techdebt/di-container-cleanup branch July 16, 2020 13:43
@nickvergessen
Copy link
Member

nickvergessen commented Jul 28, 2020 •
edited
Loading

This breaks apps ability to use overwriteService in unit tests :/
E.g. https://github.com/nextcloud/spreed/blob/31c20949b4132e0024446a30f0f3a423bf145906/tests/php/Signaling/BackendNotifierTest.php#L126 is currently failing

@MorrisJobke
Copy link
Member

This breaks apps ability to use overwriteService in unit tests :/
E.g. https://github.com/nextcloud/spreed/blob/31c20949b4132e0024446a30f0f3a423bf145906/tests/php/Signaling/BackendNotifierTest.php#L126 is currently failing

So we need to adjust overwriteService to use the new approach, right?

@nickvergessen nickvergessen mentioned this pull request Jul 28, 2020
Merged
@nickvergessen
Copy link
Member

Yeah, fixed in #22027

@ChristophWurst
Copy link
Member Author

Documented at nextcloud/documentation#5160

@ChristophWurst ChristophWurst removed the pending documentation This pull request needs an associated documentation update label Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@kesselb kesselb Awaiting requested review from kesselb

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@splitt3r splitt3r Awaiting requested review from splitt3r

Assignees

@ChristophWurst ChristophWurst

Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement integration technical debt
Projects
None yet
Milestone
Nextcloud 20
Development

Successfully merging this pull request may close these issues.
4 participants
@ChristophWurst @nickvergessen @MorrisJobke @rullzer