8000 Anonymous services do not work for global classes · Issue #22146 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

Anonymous services do not work for global classes #22146

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
curry684 opened this issue Mar 24, 2017 · 13 comments
Closed

Anonymous services do not work for global classes #22146

curry684 opened this issue Mar 24, 2017 · 13 comments

Comments

@curry684
Copy link
Contributor
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version dev-master

Using the standard Twig Extensions library, the following definitions work as expected:

services:
    _defaults:
        public: false
        tags: [ twig.extension ]

    random.name:
        class: Twig_Extensions_Extension_Intl

While the following doesn't:

services:
    _defaults:
        public: false
        tags: [ twig.extension ]

    Twig_Extensions_Extension_Intl: ~

It throws:

The definition for "Twig_Extensions_Extension_Intl" has no class. If you intend to inject this service dynamically at runtime, please mark it as synthetic=true. If this is an abstract definition solely used by child definitions, please add abstract=true, otherwise specify a class to get rid of this error.

It seems to be related to the fact that it's a class in the global namespace.
@nicolas-grekas
Copy link
Member
nicolas-grekas commented Mar 24, 2017 •
edited
Loading

True, and not a bug: only namespaced classes can have their class attribute omitted, as before 3.3.
You need to do:

Twig_Extensions_Extension_Intl:
    class: Twig_Extensions_Extension_Intl

@nicolas-grekas
Copy link
Member

Note that this looks like an argument to add namespaces to Twig :)
Closing here.

@curry684
Copy link
Contributor Author

I would've expected this exception in the blog post on the subject ;)

Just curious though - the restriction seems arbitrary to me, why would this be forbidden? The absence of the class property coupled with a simple class_exists call on the service name should work identically for both global and namespaced classes. What difference am I not seeing?

@stof
Copy link
Member
stof commented Mar 24, 2017

@curry684 this is because service ids are not required to be class names. When we see a \ in it, we assume it is a class name (this char is not used in common naming conventions for services).
Always assuming the id is a class name when you forget the class for a service definition using an arbitrary id actually hurts DX (you then get weird errors about unknown class during instantation).

@javiereguiluz can you update the blog post to add a note about the shortcut syntax requiring to use namespaces ?

@curry684
Copy link
Contributor Author
curry684 commented Mar 24, 2017 •
edited
Loading

@stof I fully agree such an error would be confusing, but there's no need to? If the class property is missing and the ID is not a loadable class we could throw a verbose mess 8000 age during compilation stating that a service definition either has to be named after a defined class, or contain a class property. That would've been much better than the DX I had this morning in the 30 minutes before I posted this issue. Something has to change on that end anyway even if we improve the docs.

Not like the current situation is all that good since I can still define services with a backslash in the name (unconventional but not forbidden) and point them to completely different classes.

@nicolas-grekas
Copy link
Member

@curry684 this (your suggestion included) has been discussed previously in #21133 (and linked pages).

@nicolas-grekas
Copy link
Member

See also #21380

@nicolas-grekas
Copy link
Member

@curry684 would you like to submit a PR to enhance the exception message (adding a class_exists + interface_exists check to make it more DX friendly?)

@curry684
Copy link
Contributor Author

I'll try to make some time for this and #22143 this weekend or early next week.

Just to be clear: I would be allowing the use case in the opening post, by first checking for class property, then backslashes, then the class/interface check. And if all fail throw a proper and verbose error during compilation describing the problem.

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Mar 24, 2017 •
edited
Loading

The service definitions should remain context free, which means we should not do a class_exists check to turn Twig_Extensions_Extension_Intl: ~ into a working service when the context allows so (what's inside vendor/).

Instead, we should just make sure the exception message thrown by CheckDefinitionValidityPass is as friendly as possible, to save you these 30 minutes you lost :) So to me, class_exists/interface_exists should just be used to construct a better exception message when applicable.

@curry684
Copy link
Contributor Author

Ok I can agree with that for now as it is the most consistent solution, I see the context argument, and fixes the DX.

As a whole my morning spent using all the new service features in 3.3-dev in a big 3.2 project did give me a bunch of icky feelings about the recent movement of the DI container, and I do think a lot of this recent stuff has to be rethought as a whole for 4.0 next year. I think the features are generally great but the cohesion within the current BC constraints is 'suboptimal'.

Just looking at this issue I see that service IDs starting with a _ are reserved now except that they aren't, _defaults has become a magic word that may or may not define a service called _defaults, services can be named with backslashes as long as they don't contain a class property, at which point they will cause weird runtime crashes, and the global namespace is sort of discriminated in service land for defendable but not to an outsider readily apparent reasons.

There's some DX to be done in DI imho :)

curry684 added a commit to curry684/symfony that referenced this issue Mar 24, 2017
@curry684
[DependencyInjection] Throw helpful error when shortcutting global cl…
8000
c57b685 
…asses

As discussed in symfony#22146 the error message received when trying to use a class in the global
namespace as a service without defined class is confusing. Helpful information was added
pointing out this current limitation.
@curry684 curry684 mentioned this issue Mar 24, 2017
Merged
@nicolas-grekas
Copy link
Member

We have two months of feat freeze to make things as best as possible. Your PR #22153 is on this path :)

@curry684
Copy link
Contributor Author

Happy to help, that's why I share my real world experiences :)

curry684 added a commit to curry684/symfony that referenced this issue Mar 26, 2017
@curry684
[DependencyInjection] Throw helpful error when shortcutting global cl…
83a786b 
…asses

As discussed in symfony#22146 the error message received when trying to use a class in the global
namespace as a service without defined class is confusing. Helpful information was added
pointing out this current limitation.
curry684 added a commit to curry684/symfony that referenced this issue Mar 26, 2017
@curry684
[DependencyInjection] Throw helpful error when shortcutting global cl…
b9e7b4f 
…asses

As discussed in symfony#22146 the error message received when trying to use a class in the global
namespace as a service without defined class is confusing. Helpful information was added
pointing out this current limitation.
fabpot added a commit that referenced this issue Mar 27, 2017
@fabpot
minor #22153 [DX] [DI] Throw more helpful error when shortcutting glo…
3cd563a 
…bal classes (curry684)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DX] [DI] Throw more helpful error when shortcutting global classes

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    |no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #22146
| License       | MIT

As discussed in #22146 the error message received when trying to use a class in the global
namespace as a service without defined class is confusing. Helpful information was added
pointing out this current limitation.

Commits
-------

b9e7b4f [DependencyInjection] Throw helpful error when shortcutting global classes
@fabpot fabpot closed this as completed Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@fabpot @nicolas-grekas @stof @curry684
0