Make the conditions in joins in dynamic queries use Condition objects

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

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.

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.

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.

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.

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.

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.

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.

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

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.

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.

Could I get a review from one of the committers, I have rebased this PR now for the seventh time.

I hear you. I rebased one for the 16th time in 3 months this morning.

Anyway, rebased again.

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. Restoring RTBC status.

@daffie:

The two snippets in the issue summary use $query->join('inner', ...) and $query->join('left', ...). I think that is a little odd, but it is all right since they are not explicitly described as equivalent.

But the first before/after example in the change record (CR) has the same examples. There, I think it is a problem: an inner JOIN is different from a left JOIN, isn't it? Or did you do that on purpose for some clever reason that I am missing?

I will not move the issue back to NW for that, but I will add an issue tag.

I have an even smaller suggestion about another example in the CR:

$condition = \Drupal::database()
  ->condition('AND')
  ->compare('d.entity_id', 'b.entity_id')
  ->compare('d.entity_type', 'b.entity_type')
  ->compare('d.field_name', 'b.field_name');

As I read it, the coding standards encourage breaking before each method if each method belongs to the same object. Here, condition() is a method on the Connection object, and the three compare() methods belong to the Condition object. So I would rewrite that snippet (saving one line) as

$condition = \Drupal::database()->condition('AND')
  ->compare('d.entity_id', 'b.entity_id')
  ->compare('d.entity_type', 'b.entity_type')
  ->compare('d.field_name', 'b.field_name');

Exciting work. I left a review on the MR.

This looks great. I just happened to stumble on the join methods allowing a condition object.

How do you do this:

> $connection->joinCondition()->compare('n.nid', 'c.nid'));

if you want to let the join() method provide the table alias automatically?

Another thing - the name 'compare' for the method makes me think of comparison operators like '<' and '>'.

Wouldn't something like 'equate' be more descriptive?