Better feature detection for DoctrineExtractor

[dunglas]

Fixes #883

[kimhemsoe]

@dunglas Alot of tests failing, you have an what is going on?

[alcaeus]

@kimhemsoe I believe #882 should fix them.

[dunglas]

@alcaeus not totally, I use PHP features that aren't supported by PHP5. I need to take a deeper look

[alcaeus]

@dunglas Could you give this another look please?

[greg0ire]

@dunglas php 5 seems to have been dropped since you started working on this, let me restart the build :)

DoctrineBundle/composer.json

Line 27 in ad278ef

"php": "^7.1",

[greg0ire]

Still failing, I'm afraid you will have to rebase.

[alcaeus]

@greg0ire This fix should go into 1.10.x, where we still have 5.5. That is unless @doctrine/team-symfony-integration agrees that we can leave this as is for 1.10?

[greg0ire]

That would mean red builds on that branch, or dropping php 5 there

[alcaeus]

With "as is" I meant "as is in 1.10.x currently", not "as is in this PR". It's probably best to completely fix it in 1.10.x as well though.

[xabbuh]

I changed the target branch which makes the build slightly look better. But it seems that the fix still needs some work (see the failing tests).

👍 for me to release 1.10.1 without this PR (and release 1.10.2 once this one is ready).

[alcaeus]

for me to release 1.10.1 without this PR (and release 1.10.2 once this one is ready).

Yeah, thought of that as well. Will do 👍

[alcaeus]

@dunglas can you take another look at this please?

[dunglas]

I'll try to take a look asap, unfortunately it's not on top of my todo list right now tbh

[alcaeus]

That's fine - I'm just not aware of what the issue is and how to fix it. Maybe @doctrine/team-symfony-integration can advise how to proceed here?

[silverbackdan]

Does this not just need a rebase now as the travis config has been updated to reflect the new php minimum requirements? I might be wrong but I think I'll try creating a PR with the new travis config and this update and see if it works to help get this resolved.

[silverbackdan]

Does this not just need a rebase now as the travis config has been updated to reflect the new php minimum requirements? I might be wrong but I think I'll try creating a PR with the new travis config and this update and see if it works to help get this resolved.

Sorry, missed the conversation regarding 1.10.x as well.

[silverbackdan]

I've just created an alternative PR where tests pass and could be merged to 1.10.x if you think it is acceptable.

[alcaeus]

Thanks @silverbackdan - I'll take a look tomorrow. @dunglas maybe you could take a quick look as well?

[alcaeus]

After taking a look, I can see why it's not at the top of your todo list. We can't reliably detect whether the Doctrine Bridge is installed in version 4.2 (or newer) by checking features. The check introduced here won't work as there is no parameter type in the DoctrineExtractor constructor, so the check will always fail.

The only check I came up with was checking the type hint of the DoctrineChoiceLoader argument (to distinguish Symfony 3 from Symfony 4) and checking for the existence of the DoctrineTransactionMiddlewareFactory class, but that matches both Symfony 4.0 and 4.2. There are no other obvious changes in the bridge that would help during version detection.

We could use a package like ocramius/package-versions to go version-based on this check, but all other checks check the code so I didn't just want to go that route.

[greg0ire]

How about using this: https://github.com/symfony/doctrine-bridge/compare/4.1...v4.2.0#diff-52766cedbcf59f925f7316471f14d51fR29 ?

Something like is_a(DoctrineType::class, ResetInterface::class, true) could work, apparently: https://3v4l.org/s0Rp9

[alcaeus]

Good catch, that would work. I'll test that later today unless somebody else wants to try it earlier.

[greg0ire]

I'm working on a PR, but will not have time to test it :P

[greg0ire]

See #917

[xabbuh]

see #918

[greg0ire]

How about an approach where we detect this property: https://github.com/symfony/doctrine-bridge/compare/4.1...v4.2.0#diff-0ddb11f82d9ee9635a016dbb98a03de9R31 ? It's supported in php 5: https://secure.php.net/manual/en/reflectionclass.hasproperty.php

[greg0ire]

BTW, I don't really understand your point, @alcaeus :

The check introduced here won't work as there is no parameter type in the DoctrineExtractor constructor, so the check will always fail.

There is a parameter type… in v 4.1 , and the code in this PR checks that.

null !== $constructorParameterType && $constructorParameterType->getName() === ClassMetadataFactory::class

This will return true when using v4.1, right?

@xabbuh told me the actual issue is with getType not existing. I'm making a PR based on my idea.

[greg0ire]

I reused #917 for this

[alcaeus]

Closing in favour of #917.