doctrine / mongodb-odm

The Official PHP MongoDB ORM/ODM
https://www.doctrine-project.org/projects/doctrine-mongodb-odm/en/latest/
MIT License
1.09k stars 504 forks source link

Deprecation of EagerCursor #1675

Closed olvlvl closed 6 years ago

olvlvl commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.

olvlvl commented 6 years ago

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 commented 6 years ago

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:

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 commented 6 years ago

Understood. Thanks for your message.

ScorpioT1000 commented 6 years ago

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

malarzm commented 6 years ago

@ScorpioT1000 trigger_error is already silenced

ScorpioT1000 commented 6 years ago

@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