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

Add ReturnTypeWillChange to core interface implementations #4734

Merged
merged 1 commit into from
Aug 2, 2021

Conversation

derrabus
Copy link
Member

Q A
Type bug
BC Break no
Fixed issues N/A

Summary

PHP 8.1 triggers a deprecation, if we implement Countable or IteratorAggregate without return types. This PR proposes to suppress that warning because we cannot fix this without breaking BC.

Signed-off-by: Alexander M. Turek <me@derrabus.de>
@morozov
Copy link
Member

morozov commented Jul 29, 2021

I don't think we should be implementing the support for PHP 8.1 here. Please migrate to 3.x. If it's challenging, let's work on improving the upgrade path.

@morozov morozov added the PHP label Jul 29, 2021
@derrabus
Copy link
Member Author

@morozov The blocker that remains is the ORM. Even if it's ready for DBAL 3 before PHP 8.1 is released, I doubt that the whole ecosystem around the ORM will be ready for it. As much as I would love to see DBAL 2 rot, I'd like to enable the PHP ecosystem to test PHP 8.1 and patching DBAL 2 would help a lot.

And there isn't really much to do, as far as I can tell. Two of the three PRs that I've opened this week are about fixing broken mocks without changing production code. I would volunteer to take care of the 2.13 branch, so you don't have to bother if you don't want to. And I would also volunteer to merge up and do the same work for DBAL 3 and 4. Deal?

@morozov
Copy link
Member

morozov commented Jul 30, 2021

I would volunteer to take care of the 2.13 branch, so you don't have to bother if you don't want to. And I would also volunteer to merge up and do the same work for DBAL 3 and 4. Deal?

I'll agree to this as an exception. The effort being put to support DBAL 2 should have been put to update ORM to support DBAL 3. DBAL 3 was released 8 months ago, so there was plenty of time to catch up. Once the support for PHP 8.1 is implemented (presumably, backported from the newer branches to avoid unnecessary merge conflicts), we'll have to have this discussion again, if some more changes like this are necessary in 2.x.

@derrabus
Copy link
Member Author

I'll agree to this as an exception.

Thank you very much!

The effort being put to support DBAL 2 should have been put to update ORM to support DBAL 3.

It's not like I haven't worked on that, see doctrine/orm#8780, doctrine/orm#8794, doctrine/orm#8796. I'd like to do more, but my time and my knowledge of ORM's internals are limited. 😞

@morozov
Copy link
Member

morozov commented Jul 30, 2021

It's not like I haven't worked on that [...]

I'm sorry if I came across that way. I didn't mean you personally, you're doing a great job as an external contributor. I meant the collective effort, primarily by the core team.

@greg0ire greg0ire merged commit 10022d9 into doctrine:2.13.x Aug 2, 2021
@greg0ire
Copy link
Member

greg0ire commented Aug 2, 2021

Thanks @derrabus !

@greg0ire greg0ire added this to the 2.13.3 milestone Aug 2, 2021
@derrabus derrabus deleted the bugfix/return-type-will-change branch August 4, 2021 22:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants