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

avoid deprecation notice from LoadDataFixturesDoctrineCommand #283

Closed
wants to merge 1 commit into from

Conversation

craue
Copy link
Contributor

@craue craue commented Jun 7, 2019

To get rid of the deprecation notice regarding Doctrine\Bundle\DoctrineBundle\Command\DoctrineCommand::getContainer(). This was missing in #280.

@yceruto
Copy link
Contributor

yceruto commented Jun 7, 2019

Thanks @craue !

All these changes (included #280) to get rid of deprecations make this bundle now incompatible with DoctrineBundle 1.9 and prior versions. For example with DoctrineBundle 1.9 you'll get an error as doctrine instance is not the expected command name and getDoctrine() method does not exist.

@craue
Copy link
Contributor Author

craue commented Jun 8, 2019

@yceruto, I wonder why this one wasn't triggered while running tests on Travis for #280.

@alcaeus
Copy link
Member

alcaeus commented Jun 11, 2019

@yceruto, I wonder why this one wasn't triggered while running tests on Travis for #280.

The tests are quite rudimentary, so there's a good chance the code is not being hit.

All these changes (included #280) to get rid of deprecations make this bundle now incompatible with DoctrineBundle 1.9 and prior versions. For example with DoctrineBundle 1.9 you'll get an error as doctrine instance is not the expected command name and getDoctrine() method does not exist.

#280 should be fine, as passing an extra argument to the constructor isn't a problem. However, this will make it incompatible with DoctrineBundle 1.9. I see two options here:

  1. make the call to getDoctrine conditional: if the method exists, use the new logic, otherwise use the old.
  2. Bump the minimum version of doctrine/doctrine-bundle to 1.10.x where the getDoctrine exists.

The first option allows us to fix this in a patch release, while the second option definitely requires a new minor release. I'd suggest using option and cleaning up the dependency in the next version.

@yceruto
Copy link
Contributor

yceruto commented Jun 11, 2019

#280 should be fine, as passing an extra argument to the constructor isn't a problem.

In #280 the $doctrine instance is passed to the parent::__construct() method:

thus, it will be validated as command name, which will fail on preg_match().

@alcaeus
Copy link
Member

alcaeus commented Jun 12, 2019

Oh, yeah. I completely missed that. I’ll fix it later today in a patch release.

@alcaeus
Copy link
Member

alcaeus commented Jun 12, 2019

Superseded by #285. Thanks for bringing this up, @craue!

@alcaeus alcaeus closed this Jun 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants