Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Describe things more precisely#10845

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

Closed
greg0ire wants to merge1 commit intosymfony:masterfromgreg0ire:meaning

Conversation

@greg0ire
Copy link
Contributor

Here, namespace is referring to that kind of thing:

App\Namespace\:    resource: '../../src/App/Namespace/*'

And it looks as if there is no word yet to name that kind of
configuration block. I propose to use namespace resource declaration,
but please do suggest better alternatives, (glob resouce declaration?).

@ogizanagi
Copy link
Contributor

ogizanagi commentedJan 5, 2019
edited
Loading

This was named "PSR-4 based service discovery" inhttps://symfony.com/blog/new-in-symfony-3-3-psr-4-based-service-discovery, but@weaverryan was worried about people not knowing about what it is in#10689 (comment).
I think we should indeed find a proper name and stick to it in the documentation.

Names I've seen so far:

  • PSR-4 service discovery
  • (service definitions) prototypes
  • Glob service discovery

Personally, I've always used the first one.
"resource declaration" might not really suit, as IMHO "resource" is more about DI config files.

greg0ire and sstok reacted with thumbs up emoji

@wouterj
Copy link
Member

wouterj commentedJan 5, 2019
edited
Loading

Isn't this way to specific? I mean, the thing is: If only one service in the container (be it configured explicitly or through PSR-4 service discovery) that implements this interface, the alias is automatically created.

As long asonly one service in the service container implements an interface (including services configured throughPSR-4 service discovery), Symfony will automatically create the alias and configuring it is not mandatory. It is however a good practice to configure it, to avoid errors when creating a new implementation.

(I've added the last sentence as a proposal, as I think it's unwanted behavior to have a broken container when creating a new class somewhere insrc/).

Thanks btw for following up the short slack discussion!

@greg0ire
Copy link
ContributorAuthor

"resource declaration" might not really suit, as IMHO "resource" is more about DI config files.

Right, that is indeed more about theresource: '../../src/App/Namespace/*' alone. I think thedeclaration part is good, because you feel it refers to some configuration block, and that block is a way to declare many services at once.

How about "glob-based service declaration", that will trigger a "PSR-4 service discovery".

Isn't this way to specific? I mean, the thing is: If only one service in the container (be it configured explicitly or through PSR-4 service discovery) that implements this interface, the alias is automatically created.

Are you 100% sure about this? We discussed about this only happening when both the interface and the class are discovered through the same discovery, cc@lucascourot

@greg0ire
Copy link
ContributorAuthor

Let me try on a fresh install.

@greg0ire
Copy link
ContributorAuthor

greg0ire commentedJan 5, 2019
edited
Loading

@wouterj when I do this, the alias disappears frombin/console debug:container

commit 87bc4b918233397545adef9546aab59c65eb0fd5 (HEAD -> master)Author: Grégoire Paris <postmaster@greg0ire.fr>Date:   Sat Jan 5 15:12:43 2019 +0100    Split into 2 discoveries, no longer worksdiff --git a/config/services.yaml b/config/services.yamlindex 5c4b417..fbe7e97 100644--- a/config/services.yaml+++ b/config/services.yaml@@ -11,11 +11,11 @@ services:         autowire: true      # Automatically injects dependencies in your services.         autoconfigure: true # Automatically registers your services as commands, event subscribers, etc.-    # makes classes in src/ available to be used as services-    # this creates a service per class whose id is the fully-qualified class name-    App\:-        resource: '../src/*'-        exclude: '../src/{DependencyInjection,Entity,Migrations,Tests,Kernel.php}'+    App\Domain\:+        resource: '../src/Domain/*'++    App\Infrastructure\:+        resource: '../src/Infrastructure/*'     # controllers are imported separately to make sure services can be injected     # as action arguments even if you don't extend any base controller class

I can publish the repository if you want to tinker with it.

Let me take this as an occasion to celebrate how fast it is to create a throwaway Symfony application to prove a point now. Flex is awesome! 🎉

sstok reacted with heart emoji

@ogizanagi
Copy link
Contributor

ogizanagi commentedJan 5, 2019
edited
Loading

I confirm too. It must be discovered by the same discovery in order to work. (seehttps://github.com/symfony/symfony/blob/5c2cee5c0fcf09d90ffd414bb8d13e023fc5ae9e/src/Symfony/Component/DependencyInjection/Loader/FileLoader.php#L78-L83 which is called for each configured discovery)

lucascourot reacted with thumbs up emoji

@wouterj
Copy link
Member

wouterj commentedJan 5, 2019
edited
Loading

Wow, alrighty. (I'm not really into Symfony core logic anymore 😞)

In general, I think people call theresource: ... line just the "resource option". I think the concept itself is called PSR-4 service discovery (at least, it was introduced in the PR and blog posts in this way).

What about using the concept name here?"As long as there is only one class implementing the interface, and that class is part of the same PSR-4 service discovery." We can then link thePSR-4 service discovery part to the section/article talking about that. That's how new terms/concepts were documented in the older versions of Symfony as well (i.e. when introducing the Service Container concept in Sf 2)

ogizanagi reacted with thumbs up emoji

@greg0ire
Copy link
ContributorAuthor

greg0ire commentedJan 5, 2019
edited
Loading

Not sure about the PSR-4 part… is this really incompatible with PSR-0? Maybe it only works with PSR-4 for now, but maybe there could be other ways of doing discovery in the future, who knows? How about dropping it? "As long as there is only one class implementing the interface, and that class is part of the same service discovery…" It still feels a bit awkward to use the concept name IMO, especially since the full name is "PSR-4 service discovery and registration". And can we really say "a discovery", or is discovery separated in several parts for which we should find a name ("discovery pass"?) ?

@greg0ire
Copy link
ContributorAuthor

slack://symfony-devs#naming suggests "{prototype,template} service definition" (credits to@ro0NL).

@greg0ire
Copy link
ContributorAuthor

greg0ire commentedJan 5, 2019
edited
Loading

screenshot_2019-01-05 naming symfony devs slack

@lucascourot
Copy link
Contributor

I think it's unwanted behavior to have a broken container when creating a new class somewhere in src/

@wouterj There are already different ways of breaking things up by adding a new class in src. For example with_instanceof. I find the single-interface autowiring feature very helpful especially when implementing ports & adapters architecture.

@greg0ire I'm ok with "prototype service definition"

greg0ire reacted with thumbs up emoji

@ogizanagi
Copy link
Contributor

There are already different ways of breaking things up by adding a new class in src. For example with _instanceof. I find the single-interface autowiring feature very helpful especially when implementing ports & adapters architecture.

Nevertheless, it's a very implicit behavior. Promoting to make this explicit by declaring the alias yourself is a good thing if you ask me.

(btw I'm personally never relying on this in a ports & adapters architecture, because I'm used to only use discovery+autowiring for infra & specific conventions for the application layer. This way the interfaces are usually not even part of the same discovery, which once again forces to make everything explicit and is a good thing to me.)

@greg0ire : "prototype service definition" is fine. It was the second in my list and it's how the feature is referenced in code & tests.

sstok and greg0ire reacted with thumbs up emoji

@greg0ire
Copy link
ContributorAuthor

It was the second in my list

Silly me, I didn't even notice! Let's go with that then!

Good points about explicit vs implicit, I don't like that implicit autowiring either, but that behavior still needs to be documented properly, and will be challenged more easily if people know it exists.

@greg0ire
Copy link
ContributorAuthor

Please review again, maybe you can phrase this better than I did.

@xabbuhxabbuh added this to the3.4 milestoneJan 9, 2019
Here, namespace is referring to that kind of thing:    App\Namespace\:        resource: '../../src/App/Namespace/*'And it looks as if there is no word yet to name that kind ofconfiguration block. Let us go with service definition prototype.
@javiereguiluz
Copy link
Member

Do you consider this PR finished? Thanks.

@greg0ire
Copy link
ContributorAuthor

If no one can think of a better wording, I do.

@javiereguiluz
Copy link
Member

Thank you Grégoire.

javiereguiluz added a commit that referenced this pull requestJan 17, 2019
This PR was submitted for the master branch but it was merged into the 3.4 branch instead (closes#10845).Discussion----------Describe things more preciselyHere, namespace is referring to that kind of thing:    App\Namespace\:        resource: '../../src/App/Namespace/*'And it looks as if there is no word yet to name that kind ofconfiguration block. I propose to use namespace resource declaration,but please do suggest better alternatives, (glob resouce declaration?).<!--If your pull request fixes a BUG, use the oldest maintained branch that containsthe bug (seehttps://symfony.com/roadmap for the list of maintained branches).If your pull request documents a NEW FEATURE, use the same Symfony branch wherethe feature was introduced (and `master` for features of unreleased versions).-->Commits-------1a4f8ff Describe things more precisely
@greg0iregreg0ire deleted the meaning branchJanuary 17, 2019 14:58
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@xabbuhxabbuhxabbuh approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

7 participants

@greg0ire@ogizanagi@wouterj@lucascourot@javiereguiluz@xabbuh@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp