Add getters to EntityQuery

ivanjaros: Any idea how to implement this ?

Streamlined comments and function naming, to use Gets and getFoo.

Also added more documentation about the return value for getConditions.

Looks good to me. My observations:

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
   @@ -167,6 +167,13 @@ public function condition($property, $value = NULL, $operator = NULL, $langcode
   +    return $this->condition->conditions();
   

   Should this not be: return $this->condition->&conditions();

2. Can you add a test for the &getConditions(). Just a simple one with ReflectionClass. To make sure that the function work as it should and to make sure it keeps on works in future.

3. +++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
   @@ -237,6 +251,13 @@ public function sort($field, $direction = 'ASC', $langcode = NULL) {
   +  public function &getSorts() {
   
   +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -135,6 +168,14 @@ public function range($start = NULL, $length = NULL);
   +  public function &getSorts();
   

   Why is there a "&" in the function name? Nothing is returned by reference. And the class variable you are getting is called "sort". So why not call the getter function getSort().

#10.3

$sorts = &$query->getSorts();
$sorts = 5;
var_dump($query->getSorts() === 5)

TRUE

That's why it has function &getSorts()

Points 10.1 and 10.2 have not been addressed.

this would be very helpful to get. Just ran into this with trying to change the autocomplete query and needing to override the whole buildEntityQuery method to just alter one condition.

10.1 and 10.2:

Short version, no - patch is correct.

We have examples of this already in core, fx

\Drupal\Core\Database\Query\SelectExtender

public function &conditions() {
  return $this->query->conditions();
}

In this case query could also be SelectExtender class since it's same interface.

We are not testing this elsewhere - we would basically be testing PHP class features.

Maybe we can finally get patches approved and committed?

Resting patch from #7 to make sure that all is still green.

Rerolled patch

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
@@ -156,6 +156,27 @@ public function getEntityTypeId() {
    * {@inheritdoc}
    */
+   public function &getConditions() {
+     return $this->condition->conditions();
+   }
...
+
+   /**
+    * {@inheritdoc}
+    */
+   public function &getSorts() {
+     return $this->sort;
+   }

On \Drupal\Core\Database\Query\Select this is called getConditions, maybe because its actually not just a getter.

Change getter name for conditions from getConditions to conditions since that's what core does various places for some reason.

LGTM.

+++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
@@ -163,6 +163,27 @@ public function getEntityTypeId() {
+   public function &conditions() {
...
+   public function getRange() {
...
+   public function &getSorts() {

I'm curious why it is conditions by getRange an getSorts?

To match the getters on https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Database%...

1. +++ b/core/lib/Drupal/Core/Entity/Query/QueryBase.php
   --- a/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   
   +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -112,6 +112,47 @@ public function condition($field, $value = NULL, $operator = NULL, $langcode = N
   
   @@ -112,6 +112,47 @@ public function condition($field, $value = NULL, $operator = NULL, $langcode = N
      public function exists($field, $langcode = NULL);
    
      /**
   

   just to comment that this is an allowed API change under the 1:1 base class rule

2. +++ b/core/lib/Drupal/Core/Entity/Query/QueryInterface.php
   @@ -112,6 +112,47 @@ public function condition($field, $value = NULL, $operator = NULL, $langcode = N
   +   * array(
   +   *   'field' => $field,
   +   *   'value' => $value,
   +   *   'operator' => $operator,
   +   * );
   ...
   +   * There will also be a single array entry of #conjunction, which is the
   

   /me wonders if we could replace this magic array guff with a value object at some stage (follow up of course)

We need a change record here

Change record added: https://www.drupal.org/node/3157856

Anyone interested in this issue may also be interested in #3001496: Add an alter hook to EntityQuery where we now have a simple patch for review. The issues can be committed independently, but the significant DX win is from the synergy of the two.

The current patch only provides getters for 3 properties, while we have setters for many for. I'm wondering if
(a) we should have getters for everything that has a setter here;
or
(b) we adopt a BC approach using metadata instead:

  /**
   * {@inheritdoc}
   */
  public function &getMetaData($key) {
    $propertyData = [
      'sort' => $this->sort,
      'count' => $this->count,
      'condition' => $this->condition,
      'aggregate' => $this->aggregate,
      'groupBy' => $this->groupBy,
      'condition_aggregate' => $this->aggregate,
      'sort_aggregate' => $this->aggregate,
      'range' => $this->range,
      'all_revisions' => $this->allRevisions,
      'latest_revisions' => $this->latestRevision,
      'access_check' => $this->accessCheck,
      'pager' => $this->pager,
    ];
    if (!isset($this->alterMetaData[$key]) && isset($propertyData[$key])) {
      return $propertyData[$key];
    }

    return isset($this->alterMetaData[$key]) ? $this->alterMetaData[$key] : NULL;
  }

We should be adding some test coverage to prove that our expectations of adding these methods are met.

Also going to ping subsystem maintainers for an opinion about whether we should be doing this.

Should this tie into the issue of using the default core Condition class instead of the class provided by the driver?

Re #40 "whether we should be doing this":

#3001496: Add an alter hook to EntityQuery has a soft dependency on this, so arguments from bojanz in favour of that also apply here.

Should this tie into the issue of using the default core Condition class instead of the class provided by the driver

I doubt it, anything implementation related like that should be the responsibility of the entity queries prepare() method, which has the job of turning the relatively abstract entity query into something actually executable.

These getters would be about facilitating modification of the abstract entity query prior to preparation/execution being invoked.

Re #42 - Is the entity module is using reflection to expose this stuff? I'd be surprised if it is

The entity module does work around the missing alter hook, it actually alters the sql/database query builder but its own conditions are then kinda entity query conditions again which it translates using its own Tables object. Pretty wild stuff, but that's all it can do without this issue and the alter hook. See #3086409-30: Provide a default query_access handler for core (maybe all?) entity types, where I figured this out recently as I've been helping with entity.module maintenance.

I need to look at the patch more closely, but I think these two issues are a blocker for getting the entity query access system into core.

Just ran into this trying to access the accessCheck property.

I don't know whether this is thought to be included with this issue here and I don't want to hijack this but IMHO there's also a method missing to get the raw sqlQuery. We have stuff like addTag and according hooks like hook_query_[tag]_alter to modify the query, but there's no way to get the additional results back. At the very least there should be an executeQuery() or sth. so we can get the full rows back on not just IDs.

Right now, we have to decide: Either create and use manual SQL queries - which is of course quite complicated due to all the JOINs, column names etc. or use the very comfortable entityQuery but then be forever restricted to what it provides. If there's just one thing we want to let the SQL server do or calculate, everything falls apart. This could be a whole lot easier if there was such a method.

Re-rolled patch from #27 for Drupal 10.6.3 - did no longer apply for me after updating cweagans/composer-patches to ^2.0