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

Remove container-interop/container-interop in favour of psr/container #177

Merged
merged 4 commits into from
Nov 21, 2022

Conversation

internalsystemerror
Copy link
Member

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC no
QA yes

Description

Closes #171

@internalsystemerror internalsystemerror force-pushed the remove-container-interop branch from 8699cb3 to 680f57b Compare July 17, 2022 14:19
composer.json Outdated Show resolved Hide resolved
docs/book/v2/advanced.md Outdated Show resolved Hide resolved
docs/book/v2/advanced.md Outdated Show resolved Hide resolved
@internalsystemerror internalsystemerror force-pushed the remove-container-interop branch 2 times, most recently from 0245605 to 8dd4c62 Compare August 21, 2022 17:12
@Ocramius Ocramius added this to the 3.6.0 milestone Oct 14, 2022
@internalsystemerror internalsystemerror changed the base branch from 3.3.x to 3.6.x October 29, 2022 18:15
@internalsystemerror internalsystemerror marked this pull request as ready for review October 29, 2022 18:43
@internalsystemerror
Copy link
Member Author

A couple of questions:

  • Should this target a new major? I don't think so, but thought it worth asking.
  • A lot of the psalm warnings were generated from the test folders and I managed to get rid of a few of them by calling static methods statically, but most are warnings that key might not exist in the array, which is exactly what's being tested so I added those to psalm-baseline.xml. Is that the best approach here? Or should we be improving the typing to make the tests redundant?

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Besides worsened type inference, this is OK as-is, @internalsystemerror 👍

Comment on lines +367 to +376
<PossiblyUndefinedArrayOffset occurrences="1">
<code>$inputSpec['validators']</code>
</PossiblyUndefinedArrayOffset>
</file>
<file src="test/Element/CheckboxTest.php">
<PossiblyInvalidMethodCall occurrences="1">
<code>getHaystack</code>
</PossiblyInvalidMethodCall>
<PossiblyUndefinedArrayOffset occurrences="1">
<code>$inputSpec['validators']</code>
</PossiblyUndefinedArrayOffset>
<UndefinedInterfaceMethod occurrences="1">
<code>getHaystack</code>
</UndefinedInterfaceMethod>
Copy link
Member

Choose a reason for hiding this comment

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

Inference regressed here: can we investigate why?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just as I was saying above (afaik) that this is what is being tested for. So I'm not sure why these are showing now and not before (my theory: better types in the docblocks in src).

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Other than the unused import LGTM!

src/Element/AbstractDateTime.php Outdated Show resolved Hide resolved
@Ocramius Ocramius removed this from the 3.6.0 milestone Nov 20, 2022
@Ocramius Ocramius changed the base branch from 3.6.x to 3.7.x November 20, 2022 22:31
Signed-off-by: Gary Lockett <[email protected]>
@internalsystemerror
Copy link
Member Author

Unused type import remove, rebased to the latest changes in 3.7.x, psalm baseline updated and dependency lockfile updated too.

Should be ready for rereview now 👍

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@Ocramius Ocramius left a comment

Choose a reason for hiding this comment

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

Let's roll with it, thanks @internalsystemerror!

@Ocramius Ocramius added this to the 3.7.0 milestone Nov 21, 2022
@Ocramius Ocramius merged commit 44b0380 into laminas:3.7.x Nov 21, 2022
@internalsystemerror internalsystemerror deleted the remove-container-interop branch November 21, 2022 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Removal of container-interop/container-interop
4 participants