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

Deprecation of EagerCursor #1675

Closed
olvlvl opened this issue Nov 21, 2017 · 11 comments
Closed

Deprecation of EagerCursor #1675

olvlvl opened this issue Nov 21, 2017 · 11 comments
Labels

Comments

@olvlvl
Copy link
Contributor

olvlvl commented Nov 21, 2017

Hi,

Today I updated my dependencies and mongodb-odm was updated from 1.1.6 to 1.2.0. Now I'm getting deprecation notices when I run my tests:

Doctrine\ODM\MongoDB\EagerCursor is deprecated - use Doctrine\ODM\MongoDB\Cursor instead.

The thing is I'm not using any of those in my code, so I'm not sure what I'm supposed to do here, and the UPGRADE file is not helping: https://github.com/doctrine/mongodb-odm/blob/master/UPGRADE-1.2.md

BTW the link to that document in the release note is broken: https://github.com/doctrine/mongodb-odm/releases/tag/1.2.0

@alcaeus
Copy link
Member

alcaeus commented Nov 21, 2017

You're implicitly using EagerCursor when using primers manually calling eagerCursor(true) on a query builder. The class itself is no longer required, but to keep BC we're still returning an instance of EagerCursor. There's nothing you need to do (except for changing instanceof checks away from it if there are some).

I've updated the broken link in the release.

@olvlvl
Copy link
Contributor Author

olvlvl commented Nov 21, 2017

This is my code:

        $builder
            ->field(Ingredient::PROPERTY_ALLERGENS)->prime(true)
            ->field(Ingredient::PROPERTY_FAMILY)->prime(true)
        ;

Are you telling me I should remove prime()? Is it taking care of the n+1 problem on its own?

BTW I'm not using eagerCursor().

@alcaeus
Copy link
Member

alcaeus commented Nov 21, 2017

Are you telling me I should remove prime()

No. As I said before:

There's nothing you need to do

BTW I'm not using eagerCursor()

You're implicitly using it by calling prime. Using reference primers requires the use of eager cursors.


Some background: previously, the Doctrine\ODM\MongoDB\EagerCursor was there to extend Doctrine\MongoDB\EagerCursor which contained all functionality relating to eager cursors (mainly exhausting the base cursor when iteration started and then serving results from memory). This changed later by making Doctrine\ODM\MongoDB\Cursor wrap any cursor, effectively making the ODM EagerCursor class obsolete. Since it was obsolete, we deprecated it for removal in 2.0 as per our BC policy.

However, somebody might check if the cursor returned by a query is an instance of EagerCursor, which is why we're still using the class to keep BC. That's why you're seeing the deprecation notices and also why you don't need to do anything.

I might also add that we're suppressing error output for the corresponding trigger_error call as is done in Symfony itself: this ensures that custom error handlers can pick up the deprecation notice (this is what Symfonys PHPUnit bridge does) but PHP does not print the error.

@olvlvl
Copy link
Contributor Author

olvlvl commented Nov 21, 2017

I'm happy to learn that Symfony is suppressing errors, but we are turning them into ErrorException. Looks like we're stuck with 1.1, unless we create a fork and change the behavior, because this error is triggered by the library itself :/

@alcaeus
Copy link
Member

alcaeus commented Nov 21, 2017

but we are turning them into ErrorException

Yeah, that might be a good idea for E_ERROR or E_USER_ERROR, but doing that for E_USER_DEPRECATED is a mistake. From the PHP documentation:

E_DEPRECATED: Run-time notices. Enable this to receive warnings about code that will not work in future versions.
E_USER_DEPRECATED: User-generated warning message. This is like an E_DEPRECATED, except it is generated in PHP code by using the PHP function trigger_error().

FWIW, we're doing pretty much the same thing Symfony does to maintain BC and make it easier for people to upgrade to new versions. Upgrading to a newer version of any Symfony component might also cause deprecation notices your app would convert to exception. This is your bug, not ours.

@alcaeus alcaeus closed this as completed Nov 21, 2017
@olvlvl
Copy link
Contributor Author

olvlvl commented Nov 21, 2017

This is your bug, not ours.

I would argue that anything I cannot fix myself is not my bug.

Thanks for your messages. I'm not sure we'll ignore E_USER_DEPRECATED as it was a great help when updating to Symphony's DI v4.0. Speaking of DI v4.0, we adapted our service definitions until there was no more errors, and now we are ready for the final release. On the contrary, with mongodb-odm, as you said yourself, there's nothing we can do.

@alcaeus
Copy link
Member

alcaeus commented Nov 21, 2017

Allow me to summarize:

  1. We deprecate a class in ODM to warn of future removal of said class.
  2. To keep BC for our users, we have to keep using the class, which triggers a silent E_USER_DEPRECATED
  3. You register an error handler that may or may not convert E_USER_DEPRECATED to an exception, apparently causing errors
  4. This is still not your bug, but ours.

Looks like we can't win this one:

  • Since we're removing the class in 2.0, it has to be deprecated (eventually)
  • Returning Doctrine\ODM\MongoDB\Cursor instead of Doctrine\ODM\MongoDB\EagerCursor can be seen as BC break as some people may have been relying on that. That's why I took the decision to keep using EagerCursor internally
  • We can't make the deprecation notice more silent than it already is

Allow me to also point out that it is your decision as to how to deal with deprecation notices: you have to actively add code to see them, and even then these are purely informational messages designed to help you upgrade when it's time to do so. We're providing a tool - the usage is up to you.

@olvlvl
Copy link
Contributor Author

olvlvl commented Nov 21, 2017

Understood. Thanks for your message.

@ScorpioT1000
Copy link

ScorpioT1000 commented Feb 13, 2018

Same problem. @alcaeus you can mute the exception when it's used internally only.

@malarzm
Copy link
Member

malarzm commented Feb 13, 2018

@ScorpioT1000
Copy link

ScorpioT1000 commented Feb 13, 2018

@malarzm it is still caught by the Symfony 3.4 using monolog level "info". The error can be muted if the cursor is constructed internally by the library, passing some flag from outer scope

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants