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

2.3 #5569

Closed
wants to merge 17 commits into from
Closed

2.3 #5569

wants to merge 17 commits into from

Conversation

ThomasLandauer
Copy link
Contributor

No description provided.

| Q             | A
| ------------- | ---
| Doc fix?      | [yes]
| New docs?     | [no]
| Applies to    | [2.3]
| Fixed tickets | []
| Q             | A
| ------------- | ---
| Doc fix?      | [yes]
| New docs?     | [yes] 
| Applies to    | [2.3]
| Fixed tickets | []
| Q             | A
| ------------- | ---
| Doc fix?      | [yes]
| New docs?     | [no]
| Applies to    | [2.3]
| Fixed tickets | []

I guess it's not necessary to do anything in __construct manually - the ArrayCollection is automatically initialized when you do php app/console doctrine:generate:entities AppBundle

Please double-check!
| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3
| Fixed tickets |
Note about nesting blocks and {% endblock NAME %}
Changed <meta http-equiv> to HTML5's <meta charset>
@@ -951,8 +951,6 @@ To relate the ``Category`` and ``Product`` entities, start by creating a
products:
targetEntity: Product
mappedBy: category
# don't forget to init the collection in the __construct() method
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 this comment is kind of usefull (and not everyone uses doctrine:generate:entities, even more it's better to never use this command).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, then please change it to:
The ArrayCollection $products is automatically initialized in the __construct() method, when you call doctrine:generate:entities

@wouterj
Copy link
Member

wouterj commented Jul 28, 2015

I like the other changes! (btw, can you please remove your merge commits from the commit history?)

@ThomasLandauer
Copy link
Contributor Author

Sorry, I'm new to GitHub and don't know where to delete what :-(
Took me a while to even figure out that I did not effectively create the Pull Request at the time I wrote the changes ;-)

@stof
Copy link
Member

stof commented Jul 28, 2015

@wouterj you can squash the commits with the GH tool anyway, so this is not a hard requirement

@wouterj
Copy link
Member

wouterj commented Jul 28, 2015

@ThomasLandauer no worries, I'll delete them when merging in that case.

@ThomasLandauer
Copy link
Contributor Author

Thanks :-)

| Q             | A
| ------------- | ---
| Doc fix?      | [yes]
| New docs?     | [no]
| Applies to    | [2.3]
| Fixed tickets |[]
@@ -97,7 +97,7 @@ from inside a controller::
->getForm();

return $this->render('default/new.html.twig', array(
'form' => $form->createView(),
'form' => $form->createView()
Copy link
Member

Choose a reason for hiding this comment

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

This should be reverted to comply with the Symfony CS.

| Q             | A
| ------------- | ---
| Doc fix?      | [yes]
| New docs?     | [no]
| Applies to    | [2.3]
| Fixed tickets | []
| Q             | A
| ------------- | ---
| Doc fix?      | [yes]
| New docs?     | [no]
| Applies to    | [2.3]
| Fixed tickets | []
"app/Resources/views/Form/fields.html.twig" didn't work for me - see next code block: "form_theme form 'AppBundle:Form:fields.html.twig'"

If this change is correct, I guess it needs to be corrected multiple times on this page.
"the form" sounded like the HTML form.

| Q             | A
| ------------- | ---
| Doc fix?      | [yes]
| New docs?     | [no]
| Applies to    | [2.3]
| Fixed tickets | []
@@ -282,7 +282,7 @@ can now re-use the form customization across many templates:

.. code-block:: html+jinja

{# app/Resources/views/Form/fields.html.twig #}
{# src/AppBundle/Resources/views/Form/fields.html.twig #}
Copy link
Member

Choose a reason for hiding this comment

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

this looks wrong to me. Best practices are to put templates in app/Resources/views

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyhow. My point is that the path used in the code exampled throughout this page is inconsistent! I don't know what the best practice is. If you don't want to change it here, then please change it in the next codeblock (and probably some others).

@wouterj
Copy link
Member

wouterj commented Dec 19, 2015

Hi @ThomasLandauer!

I've taken your commits and added a new commit to revert some changes (but I kept most of them!) and fixed the template name inconsistency. I put these commits in a new PR: #6050

So I'm going to close this PR, but your contributions are not lost and you still get the credits.

Thanks for helping us improving the documentation, I'm amazed you discovered these errors and took time to fix them! 🎄

@wouterj wouterj closed this Dec 19, 2015
@ThomasLandauer
Copy link
Contributor Author

Thanks :-)

xabbuh added a commit that referenced this pull request Dec 23, 2015
…kbook doc (ThomasLandauer, WouterJ)

This PR was merged into the 2.3 branch.

Discussion
----------

Lots of minor fixes & applying best practices to form cookbook doc

This finishes #5569

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

Commits
-------

99cdc00 Revert some changes and fix template names
2b00abe Minor clarification
3ce46f6 Fixed file path
d227ffb Fixed typo
44d19de Added example for 'required' => false
ebb1e74 Update templating.rst
d806300 Typo: Removed comma
6e41f57 Another typo
a4f0318 Fixed typo
275c079 Removed unnecessary(?) YAML hint
9cd391b Note about nesting blocks and {% endblock NAME %}
41f7e6f Changed <meta http-equiv> to HTML5's <meta charset>
@ThomasLandauer ThomasLandauer deleted the 2.3 branch August 17, 2018 09:39
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.

4 participants