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

Update extension.rst - added caution box for people trying to remove the default file with services definitions #6328

Closed
wants to merge 1 commit into from

Conversation

paolo42
Copy link

@paolo42 paolo42 commented Mar 4, 2016

Following steps from the original article isn't enough, because config.yml still tries to import services.yml from the original location.

@wouterj
Copy link
Member

wouterj commented Mar 5, 2016

Hi @paolo42!

I'm afraid I don't really agree with the changes you proposed. This document is explaining how to configure the service container from within a bundle. It doesn't mention the services.yml file. So it seems a bit strange to tell people about this. Also, there might be 10 service definitions in the services.yml file, of which just 5 are moved to a share bundle. People might get confused and remove the file, because they think it otherwise won't work.

On the other hand, I can imagine people following this guide and extracting their complete AppBundle into a shared bundle. They propably do the same thing as you, remove this file and forget to remove the imports. Before adding this to the document, I would love to hear some opinions of other people/doc maintainers. What we should consider here: Is this problem common enough to document? And is this problem difficult to debug without this mention in the documentation?

If we want to document it, I propose to make it smaller. E.g. by adding a little caution box telling "If you removed the app/config/services.yml file, make sure to also remove it from the imports key in app/config/config.yml"

@paolo42
Copy link
Author

paolo42 commented Mar 7, 2016

Hi!

I understand. The article doesn't even mention the file, and I could as well just leave it empty. Also, the caution box should be in the end of the article, not the beginning.

I think I'll wait a few days for more comments and if nothing happens, I'll push --force here the "caution box" version.

@xabbuh
Copy link
Member

xabbuh commented Mar 7, 2016

👍 to add the caution.

@paolo42 paolo42 changed the title Update extension.rst - added missing step (editing config.yml) Update extension.rst - added caution box for people trying to remove the default file with services definitions Mar 9, 2016
@paolo42
Copy link
Author

paolo42 commented Mar 9, 2016

Caution version is here. Feel free to add more comments or close the request.

.. caution::

If you removed the default file with services definitions (i. e. ``app/config/services.yml``),
make sure to also remove it from the ``imports`` key in ``app/config/config.yml``.
Copy link
Member

Choose a reason for hiding this comment

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

I think just before the "Using Configuration to Change the Services" section will be a much better location for this caution (as it's not related to the last section).

Please also note that we count the 72th character from the start of the line (so with indentation spaces included) and i. e. should be i.e..

If you can do these changes, great! Otherwise, they are quite trival so we can make them during the merge process as well.

@wouterj
Copy link
Member

wouterj commented Mar 9, 2016

Thanks for the update, @paolo42! 👍 from me

…the default file with services definitions
@xabbuh
Copy link
Member

xabbuh commented Mar 10, 2016

👍

@paolo42
Copy link
Author

paolo42 commented Mar 10, 2016

Everything should be fine now. Thanks for collaboration.

xabbuh added a commit that referenced this pull request Mar 10, 2016
…g to remove the default file with services definitions (Pavel Jurecka)

This PR was submitted for the 3.0 branch but it was merged into the 2.3 branch instead (closes #6328).

Discussion
----------

Update extension.rst - added caution box for people trying to remove the default file with services definitions

Following steps from the original article isn't enough, because config.yml still tries to import services.yml from the original location.

Commits
-------

8369fe5 Update extension.rst - added caution box for people trying to remove the default file with services definitions
xabbuh added a commit that referenced this pull request Mar 10, 2016
@xabbuh
Copy link
Member

xabbuh commented Mar 10, 2016

Thank you @paolo42 for this nice improvement. I have merged it into the 2.3 branch. That's why your pull request is shown as closed instead of merged.

@xabbuh xabbuh closed this Mar 10, 2016
xabbuh added a commit that referenced this pull request Mar 11, 2016
* 2.3:
  [#6219] some tweaks
  Point that route parameters are also Request attributes
  [#6348] some minor tweaks
  [best practices] mostly typos
  [#6275] some minor tweaks
  [quick tour] mostly typos
  remove link-local IPv6 address (fe80::1)
  [#6305] move link reference to the bottom
  Mention IvoryCKEditorBundle in the Symfony Forms doc
  [#6328] minor tweak
  Update extension.rst - added caution box for people trying to remove the default file with services definitions
  Altered single / multiple inheritance sentence
  Replace XLIFF number ids by strings
xabbuh added a commit that referenced this pull request Mar 11, 2016
* 2.7:
  [#6219] some tweaks
  Point that route parameters are also Request attributes
  [#6348] some minor tweaks
  [best practices] mostly typos
  [#6275] some minor tweaks
  [quick tour] mostly typos
  remove link-local IPv6 address (fe80::1)
  [#6305] move link reference to the bottom
  Mention IvoryCKEditorBundle in the Symfony Forms doc
  [#6328] minor tweak
  Update extension.rst - added caution box for people trying to remove the default file with services definitions
  Altered single / multiple inheritance sentence
  Replace XLIFF number ids by strings
  Rename DunglasApiBundle to ApiPlatform
xabbuh added a commit that referenced this pull request Mar 11, 2016
* 2.8:
  [#6219] some tweaks
  Point that route parameters are also Request attributes
  [#6348] some minor tweaks
  [best practices] mostly typos
  [#6275] some minor tweaks
  [quick tour] mostly typos
  remove link-local IPv6 address (fe80::1)
  [#6305] move link reference to the bottom
  Mention IvoryCKEditorBundle in the Symfony Forms doc
  [#6328] minor tweak
  Update extension.rst - added caution box for people trying to remove the default file with services definitions
  Altered single / multiple inheritance sentence
  Replace XLIFF number ids by strings
  Rename DunglasApiBundle to ApiPlatform
xabbuh added a commit that referenced this pull request Mar 11, 2016
* 3.0:
  [#6219] some tweaks
  Point that route parameters are also Request attributes
  [#6348] some minor tweaks
  [best practices] mostly typos
  Fix reference to app folder
  [#6275] some minor tweaks
  [quick tour] mostly typos
  remove link-local IPv6 address (fe80::1)
  [#6305] move link reference to the bottom
  Mention IvoryCKEditorBundle in the Symfony Forms doc
  [#6328] minor tweak
  Update extension.rst - added caution box for people trying to remove the default file with services definitions
  Altered single / multiple inheritance sentence
  Replace XLIFF number ids by strings
  Rename DunglasApiBundle to ApiPlatform
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.

3 participants