Allow doctrine/common 3 and doctrine/persistence 2 #8158
Conversation
| "doctrine/dbal": "^2.9.3", | ||
| "doctrine/event-manager": "^1.1", | ||
| "doctrine/inflector": "^1.0", |
There was a problem hiding this comment.
Using the same constraint as in doctrine/common 2 here
| @@ -65,37 +64,12 @@ abstract class OrmTestCase extends DoctrineTestCase | |||
| */ | |||
| protected function createAnnotationDriver($paths = [], $alias = null) | |||
| { | |||
| if (version_compare(Version::VERSION, '3.0.0', '>=')) { | |||
There was a problem hiding this comment.
This should actually have detected annotations v2, which no longer exists
| } else { | ||
| $reader->setDefaultAnnotationNamespace('Doctrine\ORM\Mapping\\'); | ||
| } | ||
| } |
There was a problem hiding this comment.
these 3 branches correspond to versions that should have been dropped already, it seems
greg0ire
force-pushed
the
allow-common-3-and-persistence-2
branch
from
b4b8ea5 to
7ce98f9
Compare
| @@ -24,11 +24,12 @@ | |||
| "doctrine/annotations": "^1.8", | |||
| "doctrine/cache": "^1.9.1", | |||
| "doctrine/collections": "^1.5", | |||
| "doctrine/common": "^2.11", | |||
| "doctrine/common": "^2.11 || ^3.0", | |||
There was a problem hiding this comment.
That's the dependency change I'm the most worried about. @alcaeus , can you comment on what risk it represents? I made sure persistence 2.0 is not risky to upgrade to, but I don't know about the doctrine/common 3 BC-breaks
There was a problem hiding this comment.
The old Lexer and Inflector classes in doctrine/common have already been migrated away. But we need to add the dependency on doctrine/lexer (used by the DQL lexer) instead of relying on the transitive dependency coming from doctrine/annotations IMO.
And doctrine/reflection should be checked for direct usages in the ORM too.
There was a problem hiding this comment.
That's the dependency change I'm the most worried about. @alcaeus , can you comment on what risk it represents? I made sure persistence 2.0 is not risky to upgrade to, but I don't know about the
doctrine/common3 BC-breaks
Sure! The main change is that we're dropping a bunch of dependencies that we extracted into standalone packages, e.g. annotations, lexer, collections, and so on. For any package that the ORM directly depends upon, the package should be added as a direct dependency. Grepping through the code reveals these namespaces being used:
$ grep -iroh -e "Doctrine\\\\Common\\\\[a-z]*" lib | sort | uniq
Doctrine\Common\Annotations
Doctrine\Common\Cache
Doctrine\Common\ClassLoader
Doctrine\Common\Collections
Doctrine\Common\EventArgs
Doctrine\Common\EventManager
Doctrine\Common\EventSubscriber
Doctrine\Common\Inflector
Doctrine\Common\Lexer
Doctrine\Common\Persistence
Doctrine\Common\Proxy
Doctrine\Common\Util
Doctrine\Common\Version
This gives us the following packages:
- doctrine/annotations
- doctrine/cache
- doctrine/collections
- doctrine/event-manager
- doctrine/inflector
- doctrine/lexer
- doctrine/persistence
- doctrine/common (this covers the
ProxyandUtilnamespaces).
This covers all packages dropped in 3.0, so there's no risk of someone using a transitive dependency that is no longer fulfilled.
There is a possible issue in how composer works that we noticed after releasing Inflector 2.0: composer/composer#8910. Unfortunately, there's nothing that we can do about this; simply running composer require in the wrong order will trigger this if the first dependency to be added allows ^2 || ^3, while a dependency added later does not.
It should be safe to be merged to 2.7, but if you want to be careful then you merge such a change to 2.8 only. This should take the release timeline for 2.8 into consideration.
There was a problem hiding this comment.
I pushed an amended commit with the missing dep, with the constraint it had on common 2.
This should take the release timeline for 2.8 into consideration.
From what I read, it sounds safe, and for that reason, I think we should merge in 2.7, so that we can drop the persistence ^2 || part of the constraint and the autoload black magic in 2.8, not in 2.9 or 3.0
There was a problem hiding this comment.
Works for me - any opinion @beberlei @guilhermeblanco @lcobucci?
greg0ire
force-pushed
the
allow-common-3-and-persistence-2
branch
from
7ce98f9 to
170d2c3
Compare
No description provided.