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

Updated the best practices article for reusable bundles #5014

Merged
merged 6 commits into from
Jun 26, 2015

Conversation

javiereguiluz
Copy link
Member

Q A
Doc fix? yes
New docs? no
Applies to 2.3+
Fixed tickets #3616

suffix.
A bundle is also a PHP namespace. The namespace must follow the `PSR-0`_ or
`PSR-4`_ technical interoperability standards for PHP namespaces and class names:
it starts with a vendor segment, followed by zero or more category segments, and
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 remove the serial comma before "and"

@javiereguiluz
Copy link
Member Author

@wouterj thanks for your review. I've fixed everything.

it ends with the namespace short name, which must end with a ``Bundle``
suffix.
A bundle is also a PHP namespace. The namespace must follow the `PSR-0`_ or
`PSR-4`_ technical interoperability standards for PHP namespaces and class names:
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need the word technical here?

is used to enforce uniqueness within a bundle (see below for some usage
examples).
name using underscores (``acme_blog`` for ``AcmeBlogBundle``). This alias
is used to enforce uniqueness within a bundle (see below for some usage examples).
Copy link
Member

Choose a reason for hiding this comment

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

it is also used for the bundle configuration in config files

@javiereguiluz
Copy link
Member Author

The PR has been updated with all your suggestions.

starts with a vendor segment, followed by zero or more category segments, and
it ends with the namespace short name, which must end with a ``Bundle``
suffix.
A bundle is also a PHP namespace. The namespace must follow the `PSR-0`_ or
Copy link
Contributor

Choose a reason for hiding this comment

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

bundle has a namespace?

Copy link
Member

Choose a reason for hiding this comment

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

no, bundle is in fact just a namespace or a name to refer to a location on the filesystem

@wouterj
Copy link
Member

wouterj commented Feb 22, 2015

Maybe we should add something about the symfony-bundle Composer package type?


Directory Structure
-------------------

The basic directory structure of a HelloBundle must read as follows:
The basic directory structure of an AcmeBlogBundle must read as follows (if you
use `PSR-4`_ the ``Acme/`` and ``BlogBundle/`` folders might not exist):
Copy link
Contributor

Choose a reason for hiding this comment

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

might not exists could confuse (because the content exists)? any ideas for a better wording here?

================================= ========================
``Acme\Bundle\BlogBundle`` ``AcmeBlogBundle``
``Acme\BlogBundle`` ``AcmeBlogBundle``
================================= ========================
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about shortening the table borders?

@stof
Copy link
Member

stof commented Mar 3, 2015

Other best practices for shared bundles IMO:

  • following semver
  • being registered on Packagist

├─ README.md
├─ Resources/
│ ├─ meta/
│ │ └─ LICENSE
Copy link
Member

Choose a reason for hiding this comment

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

LICENSE file may also live in the root directory directly (see #4335)

@wouterj
Copy link
Member

wouterj commented May 2, 2015

This also fixes #4335

@javiereguiluz
Copy link
Member Author

I've implemented all the changes suggested by reviewers and I've rebased it. I hope this PR can be finally ready to be merged. Thanks.

can use any other value.
* ``autoload``, this information is used by Symfony to load the classes of the
bundle. The `PSR-4`_ autoload standard is recommended for modern bundles, but
`PSR-0`_ standard is also supported.
Copy link
Member

Choose a reason for hiding this comment

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

These two lines need to be indented too to be treated as part of the list item.

@xabbuh
Copy link
Member

xabbuh commented Jun 23, 2015

@javiereguiluz There is one minor issue with the list syntax and it seems that the installation instructions for the rst format got lost when rebasing. Besides these two things the PR looks great! 👍

@javiereguiluz
Copy link
Member Author

@xabbuh I'm really sorry for having lost some information during the rebase :( I really wish I haven't lost anything else during the rebase. Thanks for your help!

* ``description``, a brief explanation of the purpose of the bundle;
* ``type``, use the ``symfony-bundle`` value;
* ``license``, ``MIT`` is the preferred license for Symfony bundles, but you
can use any other value.
Copy link
Member

Choose a reason for hiding this comment

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

should end with a semicolon

Copy link
Member

Choose a reason for hiding this comment

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

by the way, using a definition list rather than an unordered list looks more reasonable to me

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 agree. I've reworded it as a definition list. Thanks.

name and separates each word with an hyphen. For example: ``AcmeBlogBundle``
is transformed into ``blog-bundle`` and ``AcmeSocialConnectBundle`` is
transformed into ``social-connect-bundle``.
``description``
Copy link
Member

Choose a reason for hiding this comment

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

Will it render properly without a blank line between the entries?

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 don't know ... so I've added a blank line as suggested. Thanks.

@xabbuh
Copy link
Member

xabbuh commented Jun 24, 2015

👍

@weaverryan weaverryan merged commit 286631d into symfony:2.3 Jun 26, 2015
weaverryan added a commit that referenced this pull request Jun 26, 2015
… (javiereguiluz)

This PR was merged into the 2.3 branch.

Discussion
----------

Updated the best practices article for reusable bundles

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3+
| Fixed tickets | #3616

Commits
-------

286631d Added a blank line beteen definition list items
776eaee Reworded as a definition list
4276ced Added back some information wrongly removed during the rebase
fa0b902 Fixed a minor syntax issue
9981193 Minor fixes and added a cross link
a391563 Updated the best practices article for reusable bundles
weaverryan added a commit that referenced this pull request Jun 26, 2015
@weaverryan
Copy link
Member

Awesomesauce! Merged with very minor tweaks at sha: e15eb21

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.

7 participants