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

Command icon 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

joachim created an issue. See original summary.

ankitv18 made their first commit to this issue’s fork.

nishtha.pradhan made their first commit to this issue’s fork.

nishtha.pradhan’s picture

Status: Active » Needs review
vinmayiswamy’s picture

Thank 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:

/**
 * Adds a field to a database query.
 *
 * @param string $field
 *   The field to add to the query. Use dot notation to specify fields from
 *   related entities, e.g., "entity.field" or "entity:referenced_entity.field".
 *   Delta can be a numeric value or a "%delta" for any value.
 *   
 *   See {@link https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21Query%21QueryInterface.php/function/QueryInterface%3A%3Acondition/11.x} for more details.
 * @param string $type
 *   Join type, can either be INNER or LEFT.
 * @param string $langcode
 *   The language code the field values are to be queried in.
 *
 * @return string
 *   The return value is a string containing the alias of the table, a dot
 *   and the appropriate SQL column as passed in. This allows the direct use
 *   of this in a query for a condition or sort.
 *
 * @throws \Drupal\Core\Entity\Query\QueryException
 *   If $field specifies an invalid relationship.
 */
public function addField($field, $type, $langcode);

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!

smustgrave’s picture

Status: Needs review » Needs work

Seems 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.

For comprehensive documentation on the

is not found in core. Example

* See the @link themeable Default theme implementations topic @endlink for
* details.

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.

joachim’s picture

The @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.

rodrigoaguilera’s picture

Issue tags: +Barcelona2024
Novice issue reserved for the Mentored Contribution during DrupalCon Barcelona 2024. After the 27th of September 2024, this issue returns to being open to all. Thanks

This requires to change from the @link syntax to just the class name + method

sadamafridi’s picture

I'm working on this issue at DrupalCon Barcelona 2024

dksdev01’s picture

Status: Needs work » Reviewed & tested by the community

Reviewed and tested, looks good. thanks

devjuarez’s picture

I've reviewed the changes and the link has been updated from the @link syntax to just the class name + method. Looks good.

xjm’s picture

Thank 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:

    Seems 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.

    For comprehensive documentation on the

    is not found in core. Example

    * See the @link themeable Default theme implementations topic @endlink for
    * details.

    That's just a suggestion, that may fit but should checkout other examples in the repo so we can stay more inline.

    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.

  • xjm committed 9d09dbc0 on 11.x
    Issue #3458565 by nishtha.pradhan, sadamafridi, joachim, smustgrave,...

  • xjm committed 65eb6d6b on 11.0.x
    Issue #3458565 by nishtha.pradhan, sadamafridi, joachim, smustgrave,...

  • xjm committed f3d64167 on 10.4.x
    Issue #3458565 by nishtha.pradhan, sadamafridi, joachim, smustgrave,...

  • xjm committed 3fd22f7e on 10.3.x
    Issue #3458565 by nishtha.pradhan, sadamafridi, joachim, smustgrave,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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! 🦎🌊🎉

xjm’s picture

Re-crediting myself since the System Message ate it. :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.