Skip to content

Hopefully improve comments in services.yaml #285

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

Merged
1 commit merged into from
Nov 29, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions symfony/framework-bundle/3.3/config/services.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,23 @@ parameters:
services:
# default configuration for services in *this* file
_defaults:
# automatically injects dependencies in your services
Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

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

The current "before the line" comments make the section scary to me
I propose to put these comments at the end of each lines - looks better to the eye IMHO

autowire: true
# automatically registers your services as commands, event subscribers, etc.
autoconfigure: true
# this means you cannot fetch services directly from the container via $container->get()
# if you need to do this, you can override this setting on individual services
public: false
autowire: true # Automatically injects dependencies in your services.
autoconfigure: true # Automatically registers your services as commands, event subscribers, etc.
public: false # Allows optimizing the container by removing unused services; this also means
# fetching services directly from the container via $container->get() won't work.
# The best practice is to be explicit about your dependencies anyway.

# 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/*'
# you can exclude directories or files
# but if a service is unused, it's removed anyway
Copy link
Member Author

Choose a reason for hiding this comment

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

not needed anymore, esp since service removal is mentioned above now

exclude: '../src/{DataFixtures,Entity,Migrations,Tests}'
exclude: '../src/{Entity,Migrations,Tests}'

# controllers are imported separately to make sure they
# have the tag that allows actions to type-hint services
# controllers are imported separately to make sure services can be injected
# as action arguments even if you don't extend any base controller class
Copy link
Member Author

Choose a reason for hiding this comment

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

let's explain why this is needed in the first place (ie: only if you do not extend a base controller)

App\Controller\:
resource: '../src/Controller'
tags: ['controller.service_arguments']

# add more service definitions when explicit configuration is needed
# please note that last definitions always *replace* previous ones
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this last note is the most important one, as we've already got several reports from ppl who've got that wrong

Copy link
Member

Choose a reason for hiding this comment

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

Sadly I don't think this --> "please note that last definitions always replace previous ones" will be enough to understand the problem. The worst thing is that is a tricky problem and not easy to explain: if you define a controller services bellow the App\Controller\ config, it's natural to think that your service will inherit everything from above.

Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

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

that's why we absolutely need this line: to hint the user when something doesn't work as one would maybe expect

Copy link
Member

Choose a reason for hiding this comment

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

Should we include a commented-out example config below?

    # Example: override a service so you can explicitly wire a `$isDebug` constructor arg
    # App\Service\ExampleService
    #     arguments:
    #         $isDebug: '%kernel.debug%'

Copy link
Member Author

@nicolas-grekas nicolas-grekas Nov 29, 2017

Choose a reason for hiding this comment

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

not sure: we did not add examples for parameters, and I think it was a conscious decision