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

Document more BC breaks #2012

Merged
merged 5 commits into from
Apr 26, 2019
Merged

Conversation

alcaeus
Copy link
Member

@alcaeus alcaeus commented Apr 16, 2019

Q A
Type documentation
BC Break no
Fixed issues Part of #2009

Summary

This documents all BC breaks from the REMOVED category of https://gist.github.com/alcaeus/b8c1b38e297cc957e991683bef069771. It includes a BC layer for the DocumentRepository deprecation similar to how we handle ClassMetadata. @greg0ire I'd appreciate it if you could take a close look at that particular commit, I'm not entirely sure I didn't mess it up.

I'll create a separate PR to go through the other BC breaks the tool reported; I'm not sure we want to document every type hint that we added.

alcaeus added 2 commits April 15, 2019 15:59
* Introduce new iterator interface to allow people to update typehints
* Deprecate CommandCursor and Cursor class
* Remove deprecation warning from EagerCursor class

Since we're now deprecating all cursor classes, we can't really trigger deprecation warnings anymore since any read operation in MongoDB would cause deprecations that a user can't solve. Thus, we rely on other ways of deprecating these classes.
@alcaeus alcaeus added this to the 1.3.0 milestone Apr 16, 2019
@alcaeus alcaeus self-assigned this Apr 16, 2019
@alcaeus alcaeus requested review from greg0ire and malarzm April 16, 2019 07:11
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

The code looks good, not sure why 5.6 is unhappy with it 🤔

@alcaeus alcaeus force-pushed the document-more-bc-breaks branch from ac8c84e to 67be37d Compare April 16, 2019 07:29
@alcaeus
Copy link
Member Author

alcaeus commented Apr 16, 2019

The code looks good, not sure why 5.6 is unhappy with it 🤔

Coverage seems to have imported the DocumentRepository class, which means that importing Repository\DocumentRepository will not work without aliasing it.

@malarzm
Copy link
Member

malarzm commented Apr 16, 2019

I'm not sure we want to document every type hint that we added.

IMO it's just not worth it. A sentence like "We've added scalar typehints nearly everywhere" will do :)

@@ -29,6 +29,7 @@
*
* @since 1.1
* @author alcaeus <[email protected]>
* @deprecated This class is deprecated and will be removed in 2.0. You should typehint against the {@see Iterator} interface instead.
Copy link
Member

Choose a reason for hiding this comment

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

it'd be possible to emit a deprecation in the ctor

Copy link
Member Author

Choose a reason for hiding this comment

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

We still use this class extensively, which would cause deprecation notices users can't fix, which is why I haven't added that.

@@ -37,8 +38,9 @@
* For compatibility, this class also extends Doctrine\MongoDB\Cursor.
*
* @since 1.0
* @deprecated This class is deprecated and will be removed in 2.0. You should typehint against the {@see Iterator} interface instead.
Copy link
Member

Choose a reason for hiding this comment

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

Deprecation in the ctor?

Copy link
Member Author

Choose a reason for hiding this comment

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

*/
class EagerCursor extends Cursor
{
public function __construct(CursorInterface $baseCursor, UnitOfWork $unitOfWork, ClassMetadata $class)
{
@trigger_error(
Copy link
Member

Choose a reason for hiding this comment

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

Why is the notice removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@alcaeus alcaeus force-pushed the document-more-bc-breaks branch from 67be37d to c2cf938 Compare April 17, 2019 08:57
@alcaeus alcaeus requested a review from malarzm April 17, 2019 08:58
@alcaeus
Copy link
Member Author

alcaeus commented Apr 23, 2019

Added more deprecations from the list of BC breaks. BC breaks due to type hints are mostly relevant to people extending classes or implementing interfaces, so I'd like to provide this information in a separate UPGRADE document for people that are interested in those particular cases.

@alcaeus alcaeus force-pushed the document-more-bc-breaks branch from c2cf938 to 1519012 Compare April 23, 2019 08:36
@alcaeus alcaeus merged commit 8a8f3aa into doctrine:1.3.x Apr 26, 2019
@alcaeus alcaeus deleted the document-more-bc-breaks branch April 26, 2019 13:47
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.

3 participants