-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Removing some extra details from #6444 #6600
Removing some extra details from #6444 #6600
Conversation
|
||
choice_value | ||
~~~~~~~~~~~~ | ||
In the ``EntityType``, this defaults to the ``id`` of the entity, if it can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question: does "the id of the entity" refer explicitly to a property called id
or does it refer to the primary key value, whatever the property is called (and it works even for composite primary keys)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it refers to the PK, but I believe none of this works the same with composite PK's, but I'm not sure exactly what happens on that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works at the moment the identifier is in a single column (any PK or association), see https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/Form/ChoiceList/IdReader.php#L112 for more details.
👍 |
|
||
If the id cannot be read, for BC, the component checks if the class implements | ||
``__toString()`` and will use an incremental integer otherwise. | ||
In the ``EntityType``, this is overridden to use the ``id`` by default. When the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @weaverryan, I agree with those steps. However this information in choice_name
is quite important and should remain in a "note" or "caution" block IMHO.
Because as you said in your description, using this option prevents the optimization when loading entities.
Also, saying that the __toString
method or integers (as strings) should be kept since in fact, selecting choices (empty_data
, API, ...) really depends on this setting.
Note that there is some work in progress (and planned to be) where feedbacks would be very welcome: #6265 and #6446.
Besides that 👍
Ping @weaverryan Status: needs work |
This PR was merged into the 2.7 branch. Discussion ---------- Removing some extra details from #6444 See #6444 - I wanted to accomplish 2 things: 1) Ideally remove needing to duplicate the option - i.e. *try* to re-use the existing `.inc` file, as long as things remain clear. Also, the original `.inc` actually explain what the point of the option is. 2) Removing some extra low-level details that I'm not sure are understandable. The only thing I wasn't sure about was related to `choice_values`. If I'm using an API, and I want the user to be able to submit some string (e.g. the `username` for a User instead of the id), is this possible by setting this to `username`? Or will that mess up how the entities are queried? Commits ------- 2336e88 Trying to remove some duplication and some extra details
See #6444 - I wanted to accomplish 2 things:
Ideally remove needing to duplicate the option - i.e. try to re-use the existing
.inc
file, as long as things remain clear. Also, the original.inc
actually explain what the point of the option is.Removing some extra low-level details that I'm not sure are understandable.
The only thing I wasn't sure about was related to
choice_values
. If I'm using an API, and I want the user to be able to submit some string (e.g. theusername
for a User instead of the id), is this possible by setting this tousername
? Or will that mess up how the entities are queried?