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

Remove hard dependency on doctrine/common #440

Conversation

alexislefebvre
Copy link
Collaborator

Related to #434, I think that we don't need this hard dependency.

@alexislefebvre alexislefebvre added this to the 2.0 milestone Jul 17, 2018
@Jean85
Copy link
Contributor

Jean85 commented Jul 18, 2018

Isn't it used anywhere in the code?

@alexislefebvre
Copy link
Collaborator Author

I remember it only now, I think that it's used to read annotations in

* @Annotation
* @Target({"METHOD"})

So removing doctrine/common seems wrong…

@Jean85
Copy link
Contributor

Jean85 commented Jul 18, 2018

Well, that highlights that we do not have a test in place, probably...

@alexislefebvre
Copy link
Collaborator Author

doctrine/common was installed on Travis CI after I removed it: https://travis-ci.org/liip/LiipFunctionalTestBundle/jobs/405112576

So it must be a dependency of one of these dependencies: https://github.com/alexislefebvre/LiipFunctionalTestBundle/blob/d1335ad3a37c970a152b533e78ba091fa6370c81/composer.json#L18-L33

"doctrine/orm": "^2.6", requires doctrine/common so it will always be installed with current versions of Doctrine ORM.

If we don't install doctrine/orm, on the CI, we won't be able to count number of queries, I don't know how we can test the state where doctrine/common is not installed but we can still run internal tests.

@Jean85
Copy link
Contributor

Jean85 commented Jul 18, 2018

Common is a dependency of Doctrine 2.6, but I think it will be dropped in 2.7, so we cannot rely on that...

@alexislefebvre
Copy link
Collaborator Author

We may rely on https://github.com/doctrine/annotations if we only used doctrine/common to read annotations.

@alexislefebvre
Copy link
Collaborator Author

See #434 (comment): we have to require the new doctrine packages that provide features from doctrine/common that this bundle uses.

@Majkl578
Copy link

Common is a dependency of Doctrine 2.6, but I think it will be dropped in 2.7, so we cannot rely on that...

It won't, only in ORM 3.0 (due to dependency on doctrine/common's proxy generator). But it'd be a good move to depend on specific doctrine/* packages (doctrine/annotations in your case) instead of doctrine/common.

General rule of thumb: never rely on indirect dependencies - they may be dropped or bumped to a new major version without you noticing (since you have no constraint against the indirect dependency). 👍

Also I recommend https://github.com/maglnet/ComposerRequireChecker, it'll spot dependencies that are not directly depended upon.

@alexislefebvre alexislefebvre force-pushed the 2.x-remove-doctrine-common-from-composer.json branch from d1335ad to 95b0d00 Compare October 19, 2018 18:26
@alexislefebvre alexislefebvre changed the base branch from 2.x to master July 11, 2019 23:00
@alexislefebvre alexislefebvre merged commit 3879949 into liip:master Jul 11, 2019
@alexislefebvre alexislefebvre deleted the 2.x-remove-doctrine-common-from-composer.json branch July 11, 2019 23:11
@alexislefebvre alexislefebvre changed the title [2.x] Remove hard dependency on doctrine/common Remove hard dependency on doctrine/common Sep 6, 2019
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.

None yet

3 participants