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

[Best Pracitices] restore example in the "Service: No Class Parameter" section #4886

Conversation

uvoelkel
Copy link
Contributor

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets

In 1fb20e5 the 'how to NOT do it' config example in the "Best Practices -> Organizing Your Business Logic -> Service: No Class Parameter" section was changed to actually follow the recommendation.
It is especially confusing as the example in the section "Services: Naming and Format" is exactly the same (besides the comment)

@@ -140,9 +140,12 @@ the class namespace as a parameter:
# app/config/services.yml

# service definition with class namespace as parameter
parameters:
slugger.class: AppBundle\Utils\Slugger
Copy link
Member

Choose a reason for hiding this comment

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

missing indentation here

@stof
Copy link
Member

stof commented Jan 20, 2015

The best practive says Don't define parameters for the classes of your services., so it seems weird to show an example doing the opposite

@uvoelkel
Copy link
Contributor Author

Right, but the "Service: No Class Parameter" section shows an example of how it should NOT be done. At least that is what i get from the context.
The comment in the example says: # service definition with class namespace as parameter
Also the note: "This practice is cumbersome and completely unnecessary for your own services" suggests that the example shows how NOT to do it.

The right way to do it is described in the previous section as the note: "You may have noticed that the previous service definition doesn't configure the class namespace as a parameter" indicates.

@xabbuh
Copy link
Member

xabbuh commented Jan 20, 2015

@u-voelkel I agree with you. I actually blindly removed this parameter in #4519 without looking at the context here.

@stof
Copy link
Member

stof commented Jan 20, 2015

the whole paragraph should be reworded actually, because this practice is not only discouraged for your own services but also for shared bundles (Symfony 2.6+ does not expose class parameters anymore for its new services, and they will all be removed in 3.0)

@uvoelkel
Copy link
Contributor Author

maybe it should. to me it actually was clear. the only thing that got me tripping was the example. I understood it as a how not to do it example but could not find a difference to the correct one.

@weaverryan
Copy link
Member

Wow, great catch! For me, the wording seems good enough -- we're just highlighting (briefly) about the history of why people started doing this in their projects. But if someone did re-word it to be more clear, I would certainly accept it. Thanks!

@weaverryan weaverryan merged commit 0a678d2 into symfony:2.3 Jan 30, 2015
weaverryan added a commit that referenced this pull request Jan 30, 2015
… Parameter" section (u-voelkel)

This PR was merged into the 2.3 branch.

Discussion
----------

[Best Pracitices] restore example in the "Service: No Class Parameter" section

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | all
| Fixed tickets |

In 1fb20e5 the 'how to NOT do it' config example in the "Best Practices -> Organizing Your Business Logic -> Service: No Class Parameter" section was changed to actually follow the recommendation.
It is especially confusing as the example in the section "Services: Naming and Format" is exactly the same (besides the comment)

Commits
-------

0a678d2 fixed indentation
73ff5ea restore the how to NOT do it example in the "Service: No Class Parameter" section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants