Make the conditions in joins in dynamic queries use Condition objects

The patch is for D10.3

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@daffie #2 patch applied succesfully on drupal 11.x

Hide the patch files.

Big work, @daffie!

I put some comments so far; still trying to grasp the entire concept. I see views is impacted heavily, will need someone knowing it better to review it.

If we are deprecating passing a string as $conditions to Select::addJoin(), I think also ::join(), ::leftJoin() and ::innerJoin() should do the same.

If we are deprecating passing a string as $conditions to Select::addJoin(), I think also ::join(), ::leftJoin() and ::innerJoin() should do the same.

The methods ::join(), ::leftJoin() and ::innerJoin() are wrapper methods on top of Select::addJoin(). If you deprecate those on their own, you will get 2 deprecations. One from the wrapper method and one from Select::addJoin().

1. How about adding a helper method like this (untested) to the Select class:
   

     public function joinCondition(array ...$joins): ConditionInterface {
       $condition = $this->connection->condition('AND');
       foreach ($joins as $join) {
         assert(count($join) === 2 || count($join) === 3);
         $condition->compare($join[0], $join[1], $join[2] ?? '=');
       }
       return $condition;
     }
   

   that would IMHO make DX better: instead of

   $query->leftJoin($this->dataTable, 'data', $this->database->condition('AND')->compare("revision.$this->idKey", "data.$this->idKey")->compare("revision.$this->langcodeKey", "data.$this->langcodeKey"));
   

   we would use

   $query->leftJoin($this->dataTable, 'data', $query->joinCondition(
     ["revision.$this->idKey", "data.$this->idKey"],
     ["revision.$this->langcodeKey", "data.$this->langcodeKey"]
   ));
   

   Also, the $operator argument of the compare() method can be made mandatory.

2. Never mind the field vs column braindump. Still, I find the naming of compare too vague. How about compareFields?
3. I do not see a problem in getting 2 deprecation messages for #10, and really think we need to deprecate all the related methods.
4. I think we likely need to split this MR in one adding the feature and one doing the conversions, to make reviewing more palatable. But let's do that later.

@mondrake: I like your idea and I have 2 variants I would like to propose.

We create a helper method that would return the Condition object:

public function joinCondition(string $conjunction = 'AND') {
  return $this->connection->condition($conjunction);
}

The query before

$query->leftJoin($this->dataTable, 'data', $this->database->condition('AND')->compare("revision.$this->idKey", "data.$this->idKey")->compare("revision.$this->langcodeKey", "data.$this->langcodeKey"));

The query after

$query->leftJoin($this->dataTable, 'data', 
  $query->joinCondition()
    ->compare("revision.$this->idKey", "data.$this->idKey")
    ->compare("revision.$this->langcodeKey", "data.$this->langcodeKey")
);

We allow the join condition is an array of arrays with each inner array having two values that must be equal. When it is not a simple join than the Condition object must be used.

The query before

$query->leftJoin($this->dataTable, 'data', $this->database->condition('AND')->compare("revision.$this->idKey", "data.$this->idKey")->compare("revision.$this->langcodeKey", "data.$this->langcodeKey"));

The query after

$query->leftJoin($this->dataTable, 'data', [ 
  ["revision.$this->idKey", "data.$this->idKey"],
  ["revision.$this->langcodeKey", "data.$this->langcodeKey"]
]);

For 11.2 I personally like compare() more, because it is closer to condition(). But I can live with compareFields().

For 11.3 If you think that is better, then lets do that.

@mondrake: Thank you for the reviewing and helping to get this issue to land.

Hi @daffie

#12.1 - I like the first option! Keeps things open for uncommon join statements.

#12.2 - How about asking in Slack others' pov? So we don't go one way or another and then hit the wall later.

#12.3 - I do think that, but once again how about asking in Slack?

Is there a recommended way for testing this one?

Just following up if any recommended way of testing or if this needs any sign off?

Adding a note here, that in this Slack thread https://drupal.slack.com/archives/C1BMUQ9U6/p1699266961237289, there were concerns raised about adding so much deprecations (especially for contrib modules). Proposed approach was to either separate deprecations into a follow-up issue, or make the first step without deprecations - just the new functionality. It would also make the reviews here easier.

In the MR are the changes for the join and relationship plugins of the views module removed. I have created 📌 [PP-1] Make the conditions in joins in dynamic queries use Condition objects in the Views module Postponed as a followup to make the changes to the views module.

The deprecation of the use of string conditions in joins in dynamic queries has been removed from the MR and a followup has been created for the deprecation ( 📌 [PP-2] Deprecate the use of string conditions in joins in dynamic queries Postponed ).

So wasn't 100% sure how to best review this one

And of course gitlab is freezing up but one nitpicky thing I saw was

public function compare( could these new functions have a return typehint

As far as using the code I applied the patch for about an hour. Creating views with multiple relationships, created content and media, used layout builder and block layout. Just trying to make sure I touched as much as I could and nothing appeared to break.

I checked the CR and it reads well, the examples were extremely helpful so thank you for that!

For me I'm a +1 for giving this a shot but will leave in review for a bit longer for more eyes.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@smustgrave: Thank for the review.

I have added the return type hint to the new method Condition::compare().

Thanks! Think I'm going to mark it so we can have it for 10.3 (not sure the cut off). But also see it's passing all database tests.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rerolled for 11.x. Back to RTBC.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased and bacj to RTBC.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased the PR. Back to RTBC.

@daffie couple comments inline

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased and back to RTBC.