Import useful reflection classes

[greg0ire]

Other reflection classes do not seem to be used anywhere.

Classes were imported from the master branch, I hope that is ok?

[greg0ire]

So this is a circular dependency… yuck! It was a dev dependency in doctrine/reflection, not sure why… should I move it to a regular requirement?

[morozov]

Can you check what will fail if this dependency is removed? A dev dependency of one package is not necessarily a dev dependency of the one that depends on it. It might have been used by the reflection package to test other classes than the ones being carried over.

should I move it to a regular requirement?

As long as neither this package nor the reflection had a production dependency on the common package, it doesn't look like the right thing to do.

[greg0ire]

I forgot to add it at first and it failed 🙂

[morozov]

So this is where the package depends on the common library. It doesn't require it but supports it (hence the test). Would it make sense to just make the test skip if the library is not loaded and add it as a suggestion?

I believe there should be consistency between the dependencies of the code and the dependencies of the tests that cover that code. Not that the code doesn't have a dependency while the test does.

[greg0ire]

So to be clear, you would add composer require --dev doctrine/common to the CI script? In the end, the difference it would make would be that contributors would run part of the tests and would not have to download that lib, at the expense of slightly longer builds in our CI, so… I'm not really convinced but can do the change. Let's see if other contributors feel strongly one way or another?

[morozov]

So to be clear, you would add composer require --dev doctrine/common to the CI script?

Yes, this was the idea. Technically, the development of the script doesn't strictly depend on this package.

For instance, DBAL has code and the tests that depend on certain PHP extensions. If those extensions are not available, the tests are skipped. How is it different from this case?

I see what you're saying and I agree that eventually, we are adding the dependency on CI instead of having it added in the repository.

[greg0ire]

How is it different from this case?

Here, it doesn't depend on extensions. Extensions are more involving to install: you have to be root, composer can't do it for you, and in some cases you might even not have a system package for them. That's why it makes a lot of sense to proceed like this on the DBAL. doctrine/common has 2 dependencies: this very lib, and php, so it means no extra dep. It's just not worth it IMO.

[SenseException]

I can live with composer require --dev doctrine/common or adding it to the dev dependencies. I've seen both ways in different projects.

[morozov]

If I may summarize it, if there's no reason for the change, let's not make it (i.e. keep the development dependency).

[beberlei]

The other option is to copy the interface here and add a autoload-dev. Since its just a marker interface with no methods, there is no risk of breaking with or without doctrine/common.

[greg0ire]

I added the requirement under the require-dev section, so it's not like there is an impact on the users.

[greg0ire]

I should create a branch called 2.1.x from 2.0.x and target it, shouldn't I? Since master is aliased to 3:

persistence/composer.json

Line 54 in 3e22cae

"dev-master": "3.0.x-dev"

[SenseException]

I should create a branch called 2.1.x from 2.0.x and target it, shouldn't I? Since master is aliased to 3

I guess so. Is there a reason why master is already covering a 3.0 release?

[greg0ire]

I guess so. Is there a reason why master is already covering a 3.0 release?

Yeah, for instance master no longer contains the PersistentObject class. Full comparison here: 2.0.0...master