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

feature: replace container-interop/container-interop #96

Merged
merged 5 commits into from
Apr 4, 2022
Merged

feature: replace container-interop/container-interop #96

merged 5 commits into from
Apr 4, 2022

Conversation

boesing
Copy link
Member

@boesing boesing commented Jul 27, 2021

PoC

@boesing boesing force-pushed the feature/remove-container-interop branch from aa302d4 to 306df04 Compare July 27, 2021 22:29
boesing
boesing commented Jul 27, 2021
View reviewed changes
boesing
boesing commented Jul 27, 2021
View reviewed changes
Ocramius
Ocramius requested changes Jul 28, 2021
View reviewed changes
@boesing boesing force-pushed the feature/remove-container-interop branch from 1758c7e to a4e53e5 Compare July 28, 2021 07:15
@boesing boesing mentioned this pull request Jul 28, 2021
Open
@boesing
Copy link
Member Author

boesing commented Jul 28, 2021

I've also added a request to psr/container regarding this topic.
php-fig/container#37

@boesing boesing requested a review from Ocramius July 28, 2021 10:04
weierophinney
weierophinney requested changes Jul 30, 2021
View reviewed changes
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

There's a CS rule in our standard that is lowercasing class/namespace identifiers it does not recognized. I've pointed out one set, but there's more.

@weierophinney
Copy link
Member

@boesing If php-fig/container#38 gets accepted and released, how does that change this PR? I'm assuming we should wait to see what happens there?

@boesing
Copy link
Member Author

boesing commented Aug 6, 2021 •
edited
Loading

@weierophinney exactly.
If psr/container replaces interop by itself, it does not need to be done here.
Would remove the replace stuff along with the autoload for class_alias, applying the changes requested in this PR and we're done.
I would prefer not to replace interop as we do not own it but if it gets rejected, I think we can do it (even if we shouldn't).

@boesing boesing mentioned this pull request Aug 6, 2021
Closed
@weierophinney
Copy link
Member

Update for those following: see container-interop/container-interop#97 for details on the planned approach. In sum:

  • That package will do a v2.0.0 release that defines its own interfaces as class aliases of the PSR-11 interfaces.
  • We will adopt that in the next minor of this package, and, simultaneously, remove any references to container-interop internally. This will be a BC compatible, transparent change.
  • We will document that users can now refer directly to PSR-11 interfaces in their own code once they have set the new minor version as their minimum in their own application and/or library. We will also send out messages on social media and our blog to further reach for this initiative.
  • From there, we will decide whether to remove container-interop in the next minor or next major; most likely, it will be next major, as it will be a BC break for anybody upgrading from older minor versions.

This was referenced Nov 10, 2021
Closed
Closed
@Ocramius
Copy link
Member

We kinda need to move on with this, to enable psr/container:^2 compatibility.

@weierophinney I see no activity on container-interop/container-interop - do you have any leverage there?

@Ocramius Ocramius removed their request for review February 15, 2022 07:39
@Ocramius Ocramius mentioned this pull request Feb 23, 2022
Closed
@boesing

This comment was marked as off-topic.

Sign in to view
@Ocramius

This comment was marked as off-topic.

Sign in to view
@boesing

This comment was marked as off-topic.

Sign in to view
@boesing

This comment was marked as off-topic.

Sign in to view
@boesing

This comment was marked as off-topic.

Sign in to view
@Ocramius Ocramius mentioned this pull request Mar 21, 2022
Closed
@boesing boesing changed the base branch from 3.8.x to 3.11.x April 4, 2022 09:11
boesing added 3 commits April 4, 2022 11:28
@boesing
feature: replace container-interop/container-interop
0732d65 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
qa: add integration test for container interop
4756d91 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
qa: add container-interop integration test to verify that upstream …
465f122 
…can still use interop interfaces

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing force-pushed the feature/remove-container-interop branch from a4e53e5 to b6fb58a Compare April 4, 2022 09:29
@boesing boesing requested a review from weierophinney April 4, 2022 09:34
@boesing boesing requested a review from Ocramius April 4, 2022 09:34
boesing added 2 commits April 4, 2022 11:45
@boesing
qa: code-cleanup
19bd36e 
- PSR does not implement `Throwable` in 1.x versions.
- Suppress `AbstractPluginManager#__construct` error as it is not fixable without BC
- `LazyServiceFactoryTest` used old `PHPUnit_Framework_MockObject_MockObject` from ancient phpunit version

Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing
qa: apply coding standard after rebasing
9f5b839 
Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com>
@boesing boesing force-pushed the feature/remove-container-interop branch from ad80182 to 9f5b839 Compare April 4, 2022 09:45
weierophinney
weierophinney approved these changes Apr 4, 2022
View reviewed changes
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

Net removal of code, simplification internally, and immediate PSR-11 compatibility.

I am so thrilled by this! 😍

@weierophinney weierophinney added this to the 3.11.0 milestone Apr 4, 2022
@weierophinney weierophinney merged commit ecddf00 into laminas:3.11.x Apr 4, 2022
@boesing boesing deleted the feature/remove-container-interop branch April 4, 2022 20:17
@imonteiro imonteiro mentioned this pull request Mar 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weierophinney weierophinney weierophinney approved these changes

@Ocramius Ocramius Awaiting requested review from Ocramius

Assignees
No one assigned
Labels
Enhancement
Projects
None yet
Milestone
3.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@boesing @weierophinney @Ocramius @chinmoym2004