NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[DI] Fix FactoryReturnTypePassTest on PHP 5#29949

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

przemyslaw-bogusz wants to merge1 commit intosymfony:3.4fromprzemyslaw-bogusz:fix_DI_tests

Closed

[DI] Fix FactoryReturnTypePassTest on PHP 5#29949

przemyslaw-bogusz wants to merge1 commit intosymfony:3.4fromprzemyslaw-bogusz:fix_DI_tests

Conversation

Copy link

Contributor

przemyslaw-bogusz commentedJan 21, 2019

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

One of my PRs failed a Travis CI test on PHP 5 due to a syntax error resulting from loading a class with return type declarations:
https://travis-ci.org/symfony/symfony/jobs/482202091#L2865-L2867

SinceFactoryReturnTypePass works only PHP 7

symfony/src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php

Lines 38 to 43 inaca3d2c

	publicfunctionprocess(ContainerBuilder$container)
	{
	// works only since php 7.0 and hhvm 3.11
	if (!method_exists(\ReflectionMethod::class,'getReturnType')) {
	return;
	}

I modifiedFactoryReturnTypePassTest accordingly.

fixed FactoryReturnTypePassTest

112680d

carsonbot added Status: Needs Review DependencyInjection Bug labels

Jan 21, 2019

xabbuh reviewed

Jan 21, 2019

View reviewed changes

src/Symfony/Component/DependencyInjection/Tests/Compiler/FactoryReturnTypePassTest.php

		$pass =newFactoryReturnTypePass();
		$pass->process($container);

		if (method_exists(\ReflectionMethod::class,'getReturnType')) {

Copy link

Member

xabbuhJan 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Seeing this change it looks like the test was expected to pass on older PHP versions too. And since the test doesn't fail always we should probably rather investigate why they sometimes fail and why they pass under other circumstances.

Copy link

ContributorAuthor

przemyslaw-boguszJan 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yes there is an if/else clause in the 3.4 branch, however a few lines above that, there is reference toFactoryDummy fixture, that causes the syntax (parse) error, because it does not have a valid PHP 5 syntax:

symfony/src/Symfony/Component/DependencyInjection/Tests/Compiler/FactoryReturnTypePassTest.php

Line 34 inaca3d2c

$factory->setFactory([FactoryDummy::class,'createFactory']);

AppVeyor CI test also points out the same issue:
https://ci.appveyor.com/project/fabpot/symfony/builds/21760919?fullLog=true#L2048
Anyway, what can I do to help solve this issue? If there is an issue, to begin with ...

Copy link

Member

nicolas-grekas commentedJan 24, 2019

This did work before, and this file didn't change recently. I think this change is wrong and that there is an issue in#29944 instead: yes, the referenced factory class is PHP7-only. BUT service/factory classes shouldn't be loaded unless needed. That's what I would look for in#29944: why does it force loading the class now?