[DI] Improve class named services with root namespce #24145

ro0NL · 2017-09-10T20:29:04Z

Q	A
Branch?	3.3
Bug fix?	yes. IMHO.
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes/no
Fixed tickets	#...
License	MIT
Doc PR	symfony/symfony-docs#...

A continuation of #23468

Fixed error for both test cases is

The definition for "\DateTime | \My\DateTime" has no class attribute,
and appears to reference a class or interface in the global namespace.
Leaving out the "class" attribute is only allowed for namespaced classes.
Please specify the class attribute explicitly to get rid of this error.

IMHO both qualify as namespaced, making the error look weird (hence bugfix 3.3)

Yes; i know it doesnt work with get(\DateTime::class).. but that's not the point really :)

nicolas-grekas · 2017-09-11T05:52:48Z

Why?
Nobody should prefix their service name by a \\ IMHO.

ro0NL · 2017-09-11T07:25:31Z

Sorry, let me clarify. I tried wiring some legacy/historical classes with DI 3.3, i.e. \My_Awesome_Lib (poor-man namespacing).

I expected using a (leading) \ would trigger the class-name detection, as it's the special char.

Yet, by now, i more understand the tradeoff we did. Only My\Class is detected as that's the best experience with get(), i.e. get(My\Class::class).

However this isnt about get(), but about wiring a legacy codebase with the power of class named service feat. (ie. no extra boilerplate). But i could live with My_Lib: { class: 'My_Lib' } as well i guess, yet i dont see real harm with root namespace detection =/ Meaning you can always use class named services.. in a way.

Additionally this PR detects \My\Class the same way as My\Class, despite being 2 different services. Class name normalization is out-of-scope, thus could lead to comparison issues.

Contextual, where a class named service is needed, we could try to lookup both. Or forbid configuring both. I'd prefer that in favor of simply ignoring root namespaced classes (not detected as such).

Dont wanna push the change. Point is, currently, the class attribute can contain a leading \ as well, thus triggering the same class name comparison issues etc. We dont normalize that.

nicolas-grekas · 2017-09-11T08:49:47Z

I think I'm 👎 here, both for the \\ prefix trick and for any normalization (wouldn't be consistent with the removed case insensitivity.)

ro0NL · 2017-09-11T11:58:25Z

What's somewhat weird is we detect My\Class as class name, but ignore \My\Class. Both are class names to me.

wouldn't be consistent with the removed case insensitivity

Not the goal to introduce class name normalization, but could be applied where needed yes.

nicolas-grekas · 2017-09-11T12:00:16Z

\My\Class as ID, or class? If ID, we don't care really, that's not supported and nobody needs it to be...

ro0NL · 2017-09-11T12:13:05Z

as ID :) as class name is fixed by #23468. This is the same regex (spec) applied to ID.

If ID, we don't care really

Understand, but reads as "we dont care about class named service feat." :) Usecase is poor man namespacing.

Will go with My_Lib: { class: 'My_Lib' }, but i already noticed im mixing it (sometimes class: '\Foo', but also class: 'Foo'). Im used to leading \ in php world :)

So yes, eventually i think there needs to be a form of class name normalization somewhere. But it's no big deal; just tricky now and then.

nicolas-grekas · 2017-09-11T16:05:37Z

I think it's ok to require ppl with "poor man's namespaces" to add class explicitly, I suggest closing :)

ro0NL · 2017-09-11T16:55:58Z

settled 👍

DX-wise the exception may still be a bit weird; "\My\DateTime" has no class attribute .... is only allowed for namespaced classes

In PHP that's a namespaced class. Should we improve it?

stof · 2017-09-11T17:06:12Z

@ro0NL you won't ever have a class where doing getclass($object) gives you \My\DateTime. the class name is My\DateTime

ro0NL · 2017-09-11T17:12:40Z

But Definition::getClass() doesnt do that ;-) you're allowed to use both (and both work with ReflectionClass, which is why #23468 was needed).

edit: with comparison issues i mean ppl in extensions comparing to Def::getClass(). We could introduce a Def::isClass(Some::class) / getNormClass() for that BTW to do the check properly.

ro0NL · 2017-09-11T18:17:19Z

No bugs :) just never use leading slash in ID or class attr.

carsonbot added Status: Needs Review DependencyInjection Bug labels Sep 10, 2017

[DI] Improve class named services with root namespce

081b0d6

nicolas-grekas added this to the 3.3 milestone Sep 11, 2017

ro0NL closed this Sep 11, 2017

ro0NL deleted the di/class-case branch September 11, 2017 18:16

ro0NL mentioned this pull request Sep 17, 2017

Update docs about class named services symfony/symfony-docs#8403

Closed

ro0NL mentioned this pull request Jul 19, 2018

[RFC] Normalize root namespaces with DI #28006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Improve class named services with root namespce #24145

[DI] Improve class named services with root namespce #24145

[DI] Improve class named services with root namespce #24145

[DI] Improve class named services with root namespce #24145

Conversation