Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.2k
Allowed to return null for query_builder#6594
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
JonEastman commentedMay 21, 2016
| Q | A |
|---|---|
| Doc fix? | yes |
| New docs? | no |
| Applies to | 2.8 |
| Fixed tickets | #5934 |
| Q | A| ------------- | ---| Doc fix? | yes| New docs? | no| Applies to | 2.8| Fixed tickets |symfony#5934
Thanks, can you please updatethis line too? |
| @@ -202,9 +202,9 @@ query_builder | |||
| Allows you to create a custom query for your choices. See | |||
| :ref:`ref-form-entity-query-builder` for an example. | |||
| The value of this option can either be a ``QueryBuilder`` object ora Closure. | |||
| The value of this option can either be a ``QueryBuilder`` object,a Closure or null. | |||
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.
unless I'm missing something, it's not the option value that can benull. Instead, the closure can return null.
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.
@wouterj seehttps://github.com/symfony/symfony/blob/2.3/src/Symfony/Bridge/Doctrine/Form/Type/DoctrineType.php#L167 andhttps://github.com/symfony/symfony/blob/2.7/src/Symfony/Bridge/Doctrine/Form/Type/EntityType.php#L43 but this change should be done in 2.3 though
Hi@JonEastman! Thanks for your contribution, I've left 2 minor comments. |
@wouterj I made the suggested changes, let me know what you think |
👍 Looks good! |
👍 |
| When using a Closure, you will be passed the ``EntityRepository`` of the entity | ||
| as the only argument and should return a ``QueryBuilder``. If you'd like to display a list of empty entries, you can return null in the query_builder closure. | ||
| as the only argument and should return a ``QueryBuilder``. | ||
| If you'd like to display a list of empty entries, you can return ``null`` in the closure. |
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.
Unless I'm missing something whennull is returned it loads all entities.
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.
When you set the option tonull, but not when returnnull in the closure, isn't it?
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.
Why that? The only difference is that it's conditional. ?
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.
I don't think so (seehttps://github.com/symfony/symfony/pull/13990/files#diff-888009597299c21d535422fd4368bed3R72).
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.
Ok so I've missed the point of that feature, thanks for the enlightening!
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.
@xabbuh Looks likesymfony/symfony@317d30b has not made it to the core !? (neithermaster nor2.8)
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.
Looks like there was something wrong with the commit and it was fixed insymfony/symfony@eb08baa
The tests exists and passes:https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/Tests/Form/Type/EntityTypeTest.php#L212-L223
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.
This PR was merged into the 2.3 branch.Discussion----------Fixed query_builder optiondoc fix: 2.3+ref#6594 (comment)Commits-------45f586b Fixed query_builder option
| @@ -202,9 +202,10 @@ query_builder | |||
| Allows you to create a custom query for your choices. See | |||
| :ref:`ref-form-entity-query-builder` for an example. | |||
| The value of this option can either be a ``QueryBuilder`` object, a Closure or null. | |||
| The value of this option can either be a ``QueryBuilder`` object, a Closure or``null``. | |||
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.
Needs a rebase here when#6596 is merged in 2.8
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.
I'll take care of that while merging.
Thanks@JonEastman! I've merged your PR in the documentation and added a little versionadded box indicating that this is a new feature inf64fe19 (fixed the version in a later commit). |