TablesInterface::addField() doesn't document that $field can contain relationships

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

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!lib!Drupal!Core!Entity!Query!QueryInterface.php/function/QueryInterface%3A%3Acondition/8.9.x QueryInterface::condition()} for more details.
 * @param string $type
 *   Join type, can either be INNER or LEFT.
 * @param string $langcode
 *   The language code to use for multilingual queries.
 *
 * @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 = 'INNER', $langcode = NULL);

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!

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.

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.

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

I'm working on this issue at DrupalCon Barcelona 2024

Reviewed and tested, looks good. thanks

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

xjm → credited bibliophileaxe → .

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.

Adding credit for @bibliophileaxe who helped mentor the contributors who worked on this issue today.

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

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

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