Problem/Motivation
The docs say:
* @param string $field
* If it doesn't contain a dot, then an entity base field name. If it
* contains a dot, then either field name dot field column or field name dot
* delta dot field column. Delta can be a numeric value or a "%delta" for
* any value.
But you can chain through relationships, as seen in the test EntityQueryRelationshipTest:
->condition("user_id.entity.name", $this->accounts[0]->getAccountName(), '<>')
->condition("user_id.entity:user.name", $this->accounts[0]->getAccountName())
Steps to reproduce
Proposed resolution
Rather than document this fully here, we should refer to the comprehensive docs at Drupal\Core\Entity\Query\QueryInterface::condition()
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3458565
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #5
nishtha.pradhan commentedComment #6
vinmayiswamy commentedThank you @nishtha.pradhan, for your MR and for addressing the documentation for
TablesInterface::addField()in Drupal core.I've reviewed the changes provided in Drupal version 11.x. To enhance clarity, I suggest explicitly stating in the documentation that dot notation (.) can be used to specify fields involving relationships. This is consistent with practices observed in tests such as
EntityQueryRelationshipTest.For example:
I would appreciate feedback from others on this proposed change. Does this approach align with the understanding of how fields should be specified in Drupal queries? Are there any additional considerations we should take into account?
Your thoughts and suggestions would be valuable.
Thank you!
Comment #7
smustgrave commentedSeems the updates were slightly copied and paste from the summary, but don't think we should be introducing new phrase that don't already exist.
is not found in core. Example
That's just a suggestion, that may fit but should checkout other examples in the repo so we can stay more inline.
Nitpicky but good exercise, leaving the novice tag.
Comment #8
joachim commentedThe @link syntax is for linking a topic. I'm not sure it'll work with a method name.
Just giving the full class name and method will make a link on api.d.org.
Comment #9
rodrigoaguileraThis requires to change from the @link syntax to just the class name + method
Comment #10
sadamafridi commentedI'm working on this issue at DrupalCon Barcelona 2024
Comment #11
dksdev01 commentedReviewed and tested, looks good. thanks
Comment #12
devjuarez commentedI've reviewed the changes and the link has been updated from the @link syntax to just the class name + method. Looks good.
Comment #14
xjmThank you everyone for working on this issue!
We reviewed this issue LIVE at DrupalCon Barcelona 2024 at the mentored contribution sprint.
After reviewing the proposal, I think @joachim's proposal is a good one. I looked up the detailed documentation for QueryInterface::condition(), and it does indeed explain the needed information much more clearly and in greater detail.
I also looked at @vinmayiswamy's suggestion in #6. Thanks for thinking about this question! In this case, I think the linked
QueryInterface::condition()documentation also sufficiently covers that information.Finally, I considered @smustgrave's suggestion in #7:
This is also a good line of thinking! In this particular case, though, I think we should keep the wording that's in the current merge request. It's always helpful to have information for someone about why they might want to follow a link, so that they can decide whether or not it is helpful for them to view it.
Edit: I meant to mention that there are similar (although not identical) places where we say things like "Refer to FooBar::method() for the structure of allowed values" (or something to that effect.)
Adding credit for @bibliophileaxe who helped mentor the contributors who worked on this issue today.
Comment #19
xjmCommitted to 11.x. I've also backported the issue to 11.0.x, 10.4.x, and 10.3.x, because API documentation improvements are allowed in production bugfix (patch) releases of core.
Thanks everyone! 🦎🌊🎉
Comment #20
xjmRe-crediting myself since the System Message ate it. :)