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

moving MonologBundle docs to bundle repo #124

Merged
merged 4 commits into from
Jun 1, 2015
Merged

moving MonologBundle docs to bundle repo #124

merged 4 commits into from
Jun 1, 2015

Conversation

snoek09
Copy link
Contributor

@snoek09 snoek09 commented May 23, 2015

Added installation chapter.

Easy picks on bundle side from symfony/symfony-docs#4983

@snoek09 snoek09 changed the title Move docs to bundle repo moving MonologBundle docs to bundle repo May 23, 2015
@xabbuh
Copy link
Member

xabbuh commented May 23, 2015

looks good 👍

@snoek09 snoek09 changed the title moving MonologBundle docs to bundle repo [WIP] moving MonologBundle docs to bundle repo May 23, 2015
@wouterj
Copy link
Member

wouterj commented May 23, 2015

👍

-------------------------

Then, enable the bundle by adding the following line in the ``app/AppKernel.php``
file of your project:
Copy link

Choose a reason for hiding this comment

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

by adding the following line

Which line? The code example has more than a single line.

Would be better to assume the user knows what to look for in the code example and say "Then enable the bundle by adding it to a list of registered bundles in the app/AppKernel.php file of your project:"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be more clear. I would suggest changing this in the Installation Instructions template as well: http://symfony.com/doc/current/cookbook/bundles/best_practices.html#installation-instructions

Copy link
Member

Choose a reason for hiding this comment

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


.. code-block:: bash

$ composer require symfony/monolog-bundle "~2.7"

Choose a reason for hiding this comment

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

I'd prefer to not specify the version only composer require symfony/monolog-bundle is enough.

Copy link
Member

Choose a reason for hiding this comment

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

It isn't, as it means the 2.7 docs would make people install 3.0 (if that version is released) as it would just take the latest stable version if you don't specify a version. This could lead to much confusion.

Choose a reason for hiding this comment

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

ahh ok ;)

@stof
Copy link
Member

stof commented Jun 1, 2015

is there anything left here ?

@xabbuh
Copy link
Member

xabbuh commented Jun 1, 2015

Good to merge imho as @jakzal's comment has been addressed.

@stof stof changed the title [WIP] moving MonologBundle docs to bundle repo moving MonologBundle docs to bundle repo Jun 1, 2015
stof added a commit that referenced this pull request Jun 1, 2015
moving MonologBundle docs to bundle repo
@stof stof merged commit ae14e15 into symfony:master Jun 1, 2015
@snoek09 snoek09 deleted the move-docs-to-bundle-repo branch June 1, 2015 14:22
weaverryan added a commit to symfony/symfony-docs that referenced this pull request Jun 9, 2015
…ons (xabbuh)

This PR was merged into the 2.3 branch.

Discussion
----------

[Cookbook][Bundles] clarify bundle installation instructions

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | no
| Applies to    | all
| Fixed tickets | symfony/monolog-bundle#124 (comment)

Commits
-------

5a33e0e clarify bundle installation instructions
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.

6 participants