Skip to content

Added standard bundle installation instructions #4163

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

Closed
wants to merge 11 commits into from
53 changes: 53 additions & 0 deletions cookbook/bundles/best_practices.rst
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,59 @@ Extensive documentation should also be provided in the
the ``Resources/doc/`` directory; the ``Resources/doc/index.rst`` file is
the only mandatory file and must be the entry point for the documentation.

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.

"Instructions" should be upper-cased.

~~~~~~~~~~~~~~~~~~~~~~~~~

In order to ease the installation process of third-party bundles, we propose
you to use the following standardized instructions in your ``README`` file.

.. sidebar:: Installation Instructions Template
Copy link
Member

Choose a reason for hiding this comment

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

you should either add a note saying this is a rST template, or give also a Markdown template. Most projects are using Markdown for the readme on github.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. RST is the official format for the bundle documentation, but 99% of bundles hosted on GitHub use README.md instead of README.rst. I've switched the template to Markdown.

Copy link
Member

Choose a reason for hiding this comment

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

well, the best practice in the doc was written at a time where symfony.com was expected to have a repository of bundles with their doc being rendered (KnpBundles appeared as a third-party solution to this problem because the official tool announced by Fabien was not yet there, and never became a reality given we then choose composer meaning that KnpBundles and Packagist can probably be enough and people can use libraries which are not bundles). The de-facto standard then became Markdown because everyone is using Github (well, mostly at least) and it used to suck at rendering rST (3 years ago when building the Symfony2 ecosystem, the code-block directive was not supported by their rST renderer for instance) while Markdown was well rendered

Copy link
Member Author

Choose a reason for hiding this comment

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

To make myself more clear, this is what I would like to have:

doc_bundles

Copy link
Member

Choose a reason for hiding this comment

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

but ..raw does not achieve this. It let you pass unprocessed context (i.e. either html or latex or other format, which is why it asks you for a format btw: you have to provide it for each output format).
What you expect in your image is still a <pre> tag in HTML, and it is technically a code-block (so <pre><code> is more semantic).
the only difference with the rendering of ..code-block is that you don't want line numbers (this is fine, Sphinx has an option to omit them in its directive) and a different background color (and this is a matter of theming) But I could totally live with the code being on a block background, and people will be able to copy-paste it.
See http://symfony.com/doc/current/contributing/code/patches.html#make-a-pull-request and you will see that the code snippet containing the pull-request template can totally be copy-pasted (we did not even bothered about removing line numbers on it)

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right! We just need a simple .. code-block:: text directive to output the Markdown content "as is".

Copy link
Member

Choose a reason for hiding this comment

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

I find this sidebar really strange

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree! It's strange form the point of view of documentation source ... but I think it looks OK from the point of view of the documentation reader: see a screenshot of the result If you still think that this is wrong, I'll change it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it's wrong. A sidebar shows something that is not the main content, but just a little "side information". Things that are good to know, etc. (the "How is this event used in the Symfony Framework" sidebars of the HttpKernel docs are a good example of this).

In this case, it is the most important information of the complete section.

Copy link
Member Author

Choose a reason for hiding this comment

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

The sidebar is now gone.


This template assumes that your bundle is in its ``1.x`` version. If not,
change the ``"~1"`` installation version accordingly (e.g. ``"~2"``).

.. raw:: md

Installation
============

Step 1: Download the Bundle
---------------------------

Use Composer to add the bundle as a new dependency of your project:

```
Copy link
Member

Choose a reason for hiding this comment

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

```bash

seems to be more reasonable.

Copy link
Member

Choose a reason for hiding this comment

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

that's github specific markup, let's keep using standard markdown

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I thought it was part of the markdown specifications. But in this case we should keep the example as it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wouterj it's true that this started as GitHub-specific but it's generally accepted now. In addition, 99.99% of the bundles are published on GitHub. And another compelling reason: if you are able to develop a Symfony bundle, you definitely know how to manage all these Markdown formatting issues.

Copy link
Member

Choose a reason for hiding this comment

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

Thus, having syntax highlighting in the code snippets of the readme is much better. So I'm +1 for keeping them (the reason to use Markdown as format is related to Github anyway)

$ composer require <package-name> "~1"
```

This command requires to have installed Composer globally, as explained
in the [installation chapter](https://getcomposer.org/doc/00-intro.md)
of Composer documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't be "have Composer installed globally"?

Copy link
Member Author

Choose a reason for hiding this comment

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

And we could also use "to have Composer globally installed". I'm not sure what to do. We need the help of some native speaker.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @raulfraile. I would also remove everything after the comma and directly link "Composer globally installed".


Step 2: Enable the Bundle
-------------------------

Then, enable the bundle adding the following line in the `AppKernel`
class of your project:

```php
Copy link
Member

Choose a reason for hiding this comment

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

same here

<?php
// app/AppKernel.php

Copy link
Member

Choose a reason for hiding this comment

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

We can add a // ... here. What do you think?

public function registerBundles()
{
$bundles = array(
// ...

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 we can remove this blank line.

new <vendor>\<bundle-name>\<bundle-long-name>(),
);
}
Copy link
Member

Choose a reason for hiding this comment

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

The return statement is missing.

Copy link
Member

Choose a reason for hiding this comment

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

I usually add a // ... before this line too

Copy link
Member

Choose a reason for hiding this comment

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

If we add the return statements, I don't think that this is necessary. Usually you would simply add the new bundle at the end of the array and then return the $bundles array, won't you? So, having another placeholder there might lead to some confusion.

```

Optionally you can add *Step 3*, *Step 4*, etc. to explain other required
Copy link
Member

Choose a reason for hiding this comment

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

Add a comma after "optionally".

installation steps, such as registering routes or dumping assets.


Routing
-------

Expand Down