Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude SessionProgress from DI #18405

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cedric-anne
Copy link
Member

Checklist before requesting a review

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.

Description

See #18296 (comment)

Not sure it is necessary. There is no reference to this class in any service, so the DI will probably never try to instanciate it.

@AdrienClairembault
Copy link
Contributor

I see, I forgot that the container doesn't instantiate service that are not used so this is why it doesn't trigger an error here.
Not sure if this is needed then.

@AdrienClairembault
Copy link
Contributor

By the way are we looking into having a dedicated Service directory at some point to avoid having to manually register new services (or maybe we should use the exisiting DependencyInjection directory ?).

@Pierstoval
Copy link
Collaborator

By the way are we looking into having a dedicated Service directory at some point to avoid having to manually register new services (or maybe we should use the exisiting DependencyInjection directory ?).

Nope, "service" is too vague, anything can be a service, and that's exactly the reason why by default it's better to include almost everything, and to exclude non-service objects one-by-one. And if you have two, three or more of them, a subdirectory (like "DTO", "Entity", "Data", etc.) is necessary for these classes.

If you have a "Service" namespace/directory, you'll end up just like the roots of src/: hundreds of unsorted, unorganized and unarchitectured classes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants