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

tip for mapping definition #5011

Merged
merged 3 commits into from
Mar 14, 2015
Merged

tip for mapping definition #5011

merged 3 commits into from
Mar 14, 2015

Conversation

SrgSteak
Copy link
Contributor

If you follow the link from the security documentation (http://symfony.com/doc/current/book/security.html#loading-users-from-the-database), it's easy to miss the necessary mapping definition for the repository.

If you follow the link from the security documentation (http://symfony.com/doc/current/book/security.html#loading-users-from-the-database), it's easy to miss the necessary mapping definition for the repository.
@timglabisch
Copy link
Contributor

i am not sure if we really need this hint but i am 👍 with this PR

@SrgSteak
Copy link
Contributor Author

It's a common pitfall with newbies at our company ;)

@wouterj
Copy link
Member

wouterj commented Feb 19, 2015

I see how this can be missed if you aren't very used to Doctrine. 👍 for adding this tip

.. tip::

Don't forget to
:ref: `add the repository class to the mapping definition of your entity <book-doctrine-custom-repository-classes>`.
Copy link
Member

Choose a reason for hiding this comment

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

The whole block should be indented by four spaces. And can we maybe shorten the ref label a bit (maybe using "of your entity" out of it)? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shorten the ref? Good idea. But how can i change the indentation now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, never mind. Found the edit button : P

.. tip::

Don't forget to add the repository class to the mapping definition of
:ref: ` your entity <book-doctrine-custom-repository-classes>`.
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 I would change it to the following:

.. tip::
    Don't forget to add the repository class to the
    :ref:`mapping definition of your entity <book-doctrine-custom-repository-classes>`.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it

@xabbuh
Copy link
Member

xabbuh commented Feb 20, 2015

Thanks for the fast reactions @SrgSteak. It reads really well. 👍

@@ -583,6 +583,11 @@ The code below shows the implementation of the
}
}

.. tip::

Don't forget to add the repository class to the
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps here there is no cause or reasoning as to why we shouldn't forget. Maybe explain don't forget else it will not be hooked or it will just use the default class or other worst.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's needed to make this more verbose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with @wouterj. I think the "why" would go beyond the scope of this tip.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, anyone interested in the background will find the needed information in the referenced chapter.

@weaverryan weaverryan merged commit 7ae3a28 into symfony:2.3 Mar 14, 2015
weaverryan added a commit that referenced this pull request Mar 14, 2015
This PR was merged into the 2.3 branch.

Discussion
----------

tip for mapping definition

If you follow the link from the security documentation (http://symfony.com/doc/current/book/security.html#loading-users-from-the-database), it's easy to miss the necessary mapping definition for the repository.

Commits
-------

7ae3a28 improved link
38dcfc8 fixed intendation and shortened the link
4422c10 tip for mapping definition
weaverryan added a commit that referenced this pull request Mar 14, 2015
@weaverryan
Copy link
Member

I like it! Especially since it's based on actually seeing new users miss this detail. I did find one very minor tweak at sha: c4027c5

Thanks @SrgSteak!

weaverryan added a commit that referenced this pull request Mar 14, 2015
@weaverryan
Copy link
Member

Missed one of my tweaks (didn't save!). See sha: 8119e85. The extra space was why the build wasn't originally failing :)

weaverryan added a commit that referenced this pull request Mar 14, 2015
* 2.3:
  [#5015] Very small tweak
  [#5011] Adding one more fix I missed
  [#5011] Fixing minor build issue
  improved link
  fixed intendation and shortened the link
  Adding a break statement to improve the sample code
  Added an example about how to get the impersonating user object
  #4032 improved comments about em option
  tip for mapping definition
weaverryan added a commit that referenced this pull request Mar 14, 2015
* 2.6: (91 commits)
  [#5064] Minor language tweaks
  Fixing bad merge conflict (forgot to save!)
  Remove unnecessary component reference
  Correct RegisterListenersPass namespace
  Fix service id
  Switched the first example to a static constructor method
  added some more components for Tobion as a merger
  Fixed variable name in : Reference -> validation constraints -> count -> basic usage -> PHP
  [#5036] Typo fix (probably mine originally) caught by xabbuh
  reword to serves
  Adding a link to define "lts"
  Better wording
  Minor improvement for symfony-installer with LTS
  Updating for new security service names in 2.6
  [#5033] Tweaking variable name to "match" the service id
  [#5017] Minor language tweaks
  [#5015] Updating the security service name for 2.6 - thanks to Cordoval
  [#5015] Very small tweak
  [#5011] Adding one more fix I missed
  [#5011] Fixing minor build issue
  ...

Conflicts:
	book/security.rst
weaverryan added a commit that referenced this pull request Mar 14, 2015
* 2.7: (103 commits)
  Backporting some stuff from 2.7, that I think must have gotten merged only there by accident
  [#5064] Minor language tweaks
  Fixing bad merge conflict (forgot to save!)
  Remove unnecessary component reference
  Correct RegisterListenersPass namespace
  Fix service id
  Switched the first example to a static constructor method
  added some more components for Tobion as a merger
  Fixed variable name in : Reference -> validation constraints -> count -> basic usage -> PHP
  [#5036] Typo fix (probably mine originally) caught by xabbuh
  reword to serves
  Adding a link to define "lts"
  Better wording
  Minor improvement for symfony-installer with LTS
  Updating for new security service names in 2.6
  [#5033] Tweaking variable name to "match" the service id
  [#5017] Minor language tweaks
  [#5015] Updating the security service name for 2.6 - thanks to Cordoval
  [#5015] Very small tweak
  [#5011] Adding one more fix I missed
  ...
@xabbuh
Copy link
Member

xabbuh commented Mar 16, 2015

Nice catch @weaverryan! :)

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.

6 participants