Symfony Best Practices typos and errors

[javiereguiluz]

In a few days we are going to add the "Best Practices" book to the symfony-docs repo. That's why it will be great to collect in this issue the typos and errors found by the community. We are not talking (yet) about discussing best practices, but about actual and objective errors.

[javiereguiluz]

Don't recommend to use parameter classes for third-party bundles. Reported by @stof via Twitter

UPDATE: This error has been already fixed

[javiereguiluz]

Listing 5-5: use $postSlug instead of $slug in: ->findOneBy(array('slug' => $slug));. Reported by @aitboudad via Twitter

UPDATE: This error has been already fixed

[javiereguiluz]

Typo page 17: "You may have have noticed". Reported by @aitboudad via Twitter

UPDATE: This error has been already fixed

[cordoval]

number the suggestions on the guide

[javiereguiluz]

@cordoval very nice suggestion! @wouterj made it to us earlier and I think it will be nice to have the complete listing of all best-practices.

[cordoval]

"that are used as services are good examples of business logic stuff." try to avoid stuff 👶

[cordoval]

"meant to be shared, then you may define parameters But if you're developing a service for your own" missing period

worth perhaps mentioning to avoid this usage as it will be deprecated in next versions, so not really a practice or an option imo

[cordoval]

http://elnur.pro/symfony-without-bundles/
http://php-and-symfony.matthiasnoback.nl/2014/06/don-t-use-annotations-in-your-controllers/

the use of annotations is very contested "Annotations are by far the most convenient and agile way of setting up and looking for mapping" then later you say it is a matter of taste, maybe doing the "if you are advanced" would be more correct.

but again not focusing on the practices just typos, i mean to have a uniform phrasing here to be consistent with how it is being done in the previous suggestions

[cordoval]

1 $ composer require "doctrine/doctrine-fixtures-bundle":"~2"

can be just

1 $ composer require "doctrine/doctrine-fixtures-bundle"

and composer will find the latest on its own

[cordoval]

"Pre and Post Hooks" is very short, i know the cookbook details most of it, but it does not elaborate on simple use cases where this can be needed or why one would want those. Maybe just mentioning something like "this is used for example when one wants to ascertain authorization of some kind or something"...

[javiereguiluz]

Reword the reasoning of the recommendation of not using @Template() annotation. Reported by @aitboudad via Twitter

[javiereguiluz]

It should be parameters.yml.dist and not parameters.dist.yml. Reported by @Baptouuuu via Twitter

[cordoval]

introduce the word "smoke tests" which i think the "functional tests" referred rather are, i do this in projects but contrary to this i get the routes from the router and eliminate some routes via a blacklist

[javiereguiluz]

---

UPDATE All the errors reported previously to this comment, are now fixed. They will appear in the new document published in the coming days. Please, keep reporting typos and errors.

---

[weaverryan]

@javiereguiluz on the main page /best-practices page, we should clearly define that these are for non-reused bundles. I got feedback on Twitter that someone hadn't seen that - they suggested a <blink> tag ;)

[arkste]

What about some short Caching- & Performance-"Best Practices"?

[linkesch]

Generating a bundle without a vendor is not possible in < 2.6 (tried in 2.5.4 and 2.5.5).

php app/console generate:bundle --namespace=AppBundle --dir=src --format=annotation --no-interaction

returns

[InvalidArgumentException]
  The namespace must contain a vendor namespace (e.g. "VendorName\AppBundle" instead o
  f simply "AppBundle").
  If you've specified a vendor namespace, did you forget to surround it with quotes (in
  it:bundle "Acme\BlogBundle")?

[weaverryan]

@orthes Thanks for the report - we've got that covered in sensiolabs/SensioGeneratorBundle#290. If you find any other things like this, let us know!

[apfelbox]

(Prelude: I really like the document, nice work and great idea!)

A small note about the code example of the markdown twig extension. You can skip the method definition in the markdown twig service:

class AppExtension extends \Twig_Extension
{
    private $parser;

    public function __construct(Markdown $parser)
    {
        $this->parser = $parser;
    }

    public function getFilters()
    {
        return array(
            new \Twig_SimpleFilter(
                'md2html',
                array($this, 'markdownToHtml'),
                array('is_safe' => array('html'))
            ), 
        );
    }

    public function markdownToHtml($content)
    {
        return $this->parser->toHtml($content);
    }

    public function getName()
    {
        return 'app_extension';
    }
}

This can be simplified to

class AppExtension extends \Twig_Extension
{
    private $parser;

    public function __construct(Markdown $parser)
    {
        $this->parser = $parser;
    }

    public function getFilters()
    {
        return array(
            new \Twig_SimpleFilter(
                'md2html',
                array($this->parser, 'toHtml'),
                array('is_safe' => array('html'))
            ), 
        );
    }

    public function getName()
    {
        return 'app_extension';
    }
}

This has the additional benefit, that you don't need to copy all possible arguments and default values from Markdown::toHtml down into your own method. It has the disadvantage, that you can't "hide" arguments or modify the content. But if you just want to use the method, this is as simple as it gets.

Not sure if this is too confusing, though.

[apfelbox]

In Listing 9-9:

if ($attribute == self::CREATE && in_array(ROLE_ADMIN, $user->getRoles())) {
    return true;
}

The ROLE_ADMIN should be a string: 'ROLE_ADMIN'.

EDIT you should also add the third boolean parameter to in_array, otherwise you have a serious security issue:

$ php -r 'var_dump(in_array("ROLE_ADMIN", [0]));'
bool(true)

[dbu]

section "Using Expressions for Complex Security Restrictions" has a listing with Route annotation and Security annotation, but no ParamConverter. the description says "Notice that this requires the use of the ParamConverter..." => the code example should also have that annotation for completeness.

[javiereguiluz]

@apfelbox thanks for your comments! Regarding your first proposal, it's more concise and convenient, but I have the same concerns as you: "this may be too confusing". Please, let us think over it.

Regarding your second proposal, it's a no-brainer and we have just updated the document. It will be published after the next deploy.

@dbu let me explain the confusion. The text says that this requires ParamConverter to work ... but it's not necessary to explicitly define a @ParamConverter annotation. Type-hinting the Post class is enough for this feature to work. Thanks for the other comment about the missing @Route annotation import. This is now included and is much better this way. Thank you!

[dbu]

ah, you caught me there. i should use the ParamConverter sometimes,
sounds really nice.

[ghost]

Typo on Page 13: "available from places wih access"

[cordoval]

@richardmiller notice the remove of inflammatory language http://richardmiller.co.uk/2014/10/14/the-new-symfony-best-practices/

[javiereguiluz]

The book is now ready to be added to the symfony-docs repo: #4327

Please don't add comments about errors in that Pull Request to make it simpler for the documentation managers to merge it. Once the book is merged, please add any comment that you like. Thank you all for your help spotting errors.

[hellomedia]

Where can we add suggestions at this stage ?
I was wondering if there could be a few extra suggestions on translation keys naming conventions. ie : I see ´label.username´ and ´title.post_list´. Can we have broader recommendations ? What other categories would be recommended ? ´button.xxx´ , ´menu.xxx´ ? What about longer text ?
NB: I see the point for categories ( label, title .... ) but obviously it can also lead to duplicates.

[weaverryan]

Hey there!

Feel free to open a pull request or issue on this repository :). I would love more examples of good translation keys personally.

Cheers!