Movatterモバイル変換

src/Symfony/Component/DependencyInjection/Loader/FileLoader.php Outdated

		try {
		$r =$this->container->getReflectionClass($class);
		}catch (\ReflectionException$e) {
		$classes[$class] =$e->getMessage();

Copy link

Member

Choose a reason for hiding this comment

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

It's a little awkward to return an array with the error as the key, but since this method is private, it seems safe.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Actually, the class is the key here. I agree that the structure I'm using now is a bit more awkward than the old one, but as you said, the method is private, so it's a purely internal data structure.

That being said, if you have a suggestion regarding a better data structure, I'm all ears. 😃

src/Symfony/Component/DependencyInjection/Loader/FileLoader.php Outdated

		foreach ($classesas$class) {
		foreach ($classesas$class =>$errorMessage) {
		if (null !==$errorMessage) {
		$definition =newDefinition($class);

Copy link

Member

Choose a reason for hiding this comment

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

This is a nit-pick, but what happens of$class is actually an alias? Iimagine it would make no difference: if that Definition is ultimately used (likely due to autowiring), it would throw the error.

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

I don't expect this case to be any different than with an alias to a non-broken class. But I'll test it.

src/Symfony/Component/DependencyInjection/Loader/FileLoader.php Outdated

		foreach ($classesas$class =>$errorMessage) {
		if (null !==$errorMessage) {
		$definition =newDefinition($class);
		$definition->addError($errorMessage);

Copy link

Member

Choose a reason for hiding this comment

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

We need to think about this error message holistically :). Currently, the user will see:

Class App\Foo\MissingParentClass not found

I think we need to add more... as much as possible really. Something like:

Error when loading registering services from resource "../src/*": an error was thrown when processing the class "App\Foo\ActualClass": "Class App\Foo\MissingParentClass not found".

I would LOVE to even say the line number in YAML or XML... but wedefinitely do not know that :p (pipe dream for someday?)

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

👍 This code was more or less meant as a POC. I'll see what I can do about improving the DX.

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Copy link

Member

weaverryan commentedJan 26, 2018

Well-done@derrabus!

The question is: feature or bug-fix? It's asafe change: I mean, anyone with bad classes is currently getting an error... and this just makes itnot an error. I vote bug fix - we'll see what other people think.

Copy link

Contributor

alcaeus commentedJan 26, 2018

Agree on it being a bugfix. It doesn't introduce a new public-facing API, it just fixes incorrect behavior.

nicolas-grekas added this to the4.1 milestone

nicolas-grekas reviewed

src/Symfony/Component/DependencyInjection/Loader/FileLoader.php Outdated


		foreach ($classesas$class) {
		foreach ($classesas$class =>$errorMessage) {
		if (null !==$errorMessage) {

Copy link

Member

nicolas-grekasJan 26, 2018

Choose a reason for hiding this comment

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

what about putting this "if" just after the existing$this->setDefinition($class, unserialize($serializedPrototype)); line?
no need to explicitly create a new definition, the prototype-based one is better, isn't it?

Copy link

MemberAuthor

derrabusJan 26, 2018

Choose a reason for hiding this comment

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

I agree about the prototype definition. About moving this block down: I'd like to avoid theinterface_exists() call below, so the block should probably stay here.

Copy link

Member

nicolas-grekasJan 26, 2018

Choose a reason for hiding this comment

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

I'd like to avoid the interface_exists() call below

is there any reason for that? I'm not sure this is a valid concern :)

Copy link

MemberAuthor

derrabusJan 26, 2018

Choose a reason for hiding this comment

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

I expected theinterface_exists call to trigger the autoloader again, but appearently it doesn't. I'm moving the block.

Copy link

Member

stofJan 26, 2018

Choose a reason for hiding this comment

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

The call usesfalse as second argument, precisely to avoid the autoloading call.

Copy link

MemberAuthor

derrabusJan 26, 2018

Choose a reason for hiding this comment

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

Exactly. Block moved. ✅

derrabus force-pushed thedi-psr4-missing-parent branch fromd0f53e3 to08e17ebCompare

January 26, 2018 13:14

nicolas-grekas approved these changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels

Copy link

MemberAuthor

derrabus commentedJan 26, 2018•
edited
Loading

@nicolas-grekas,@stof: Do you agree with@weaverryan on turning this PR into a bugfix? I'd try to apply my patch against 3.4 then.

Copy link

Member

nicolas-grekas commentedJan 26, 2018

I do, go for 3.4 on my side

nicolas-grekas modified the milestones:4.1,3.4

derrabus force-pushed thedi-psr4-missing-parent branch from08e17eb to12d93fcCompare

January 26, 2018 13:37

derrabus changed the base branch frommaster to3.4

January 26, 2018 13:37

derrabus force-pushed thedi-psr4-missing-parent branch from12d93fc tobc34213Compare

January 26, 2018 13:45

Copy link

MemberAuthor

derrabus commentedJan 26, 2018

I've added some context to the error message extracted from the exception. Please have a look@weaverryan.

Copy link

MemberAuthor

derrabus commentedJan 26, 2018•
edited
Loading

I've set up asymfony/symfony-standard clone with a class that extends a missing class. With Symfony 3.4 from composer, invoking the console fails as expected.

When I link the project to the code of this PR, the console is executed without failure. Now, if I add a public alias to the service discovered from my broken class, the console fails again.

derrabus force-pushed thedi-psr4-missing-parent branch frombc34213 toba512d9Compare

January 26, 2018 13:55

Copy link

MemberAuthor

derrabus commentedJan 26, 2018•
edited
Loading

The AppVeyor failure looks unrelated.

dunglas approved these changes

weaverryan reviewed

src/Symfony/Component/DependencyInjection/Loader/FileLoader.php Outdated

		$r =$this->container->getReflectionClass($class);
		}catch (\ReflectionException$e) {
		$classes[$class] =sprintf(
		'While discovering services from namespace "%s", an error was thrown when processing the class "%s": %s',

Copy link

Member

weaverryanJan 26, 2018

Choose a reason for hiding this comment

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

nit pick :). I think we usually try to end errors with a period. So the end of this could be:

... processing the class "%s": "%s".

Copy link

MemberAuthor

derrabusJan 26, 2018

Choose a reason for hiding this comment

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

✅

weaverryan approved these changes

Copy link

Member

weaverryan left a comment

Choose a reason for hiding this comment

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

Nice job!

Don't stop PSR-4 service discovery if a parent class is missing.

3d6c3ba

derrabus force-pushed thedi-psr4-missing-parent branch fromba512d9 to3d6c3baCompare

January 26, 2018 20:16

chalasr added DependencyInjection Bug and removed Feature labels

chalasr approved these changes

Jan 27, 2018

fabpot approved these changes

Copy link

Member

fabpot commentedJan 29, 2018

Thank you@derrabus.

fabpot merged commit3d6c3ba intosymfony:3.4

fabpot added a commit that referenced this pull request

bug#25932Don't stop PSR-4 service discovery if a parent class is mi…

a956a76

…ssing (derrabus)This PR was merged into the 3.4 branch.Discussion----------Don't stop PSR-4 service discovery if a parent class is missing| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#25929| License       | MIT| Doc PR        | N/ACommits-------3d6c3ba Don't stop PSR-4 service discovery if a parent class is missing.

derrabus deleted the di-psr4-missing-parent branch

January 29, 2018 10:12

fabpot mentioned this pull request

Release v3.4.4#25956

Merged

derrabus mentioned this pull request