Skip to content
Snippets Groups Projects

Make the conditions in joins in dynamic queries use Condition objects

Open daffie requested to merge issue/drupal-3398773:3398773-make-the-conditions into 11.x

Closes #3398773

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
127 127 return $this;
128 128 }
129 129
130 /**
131 * Compare two database fields with each other.
  • Terminology - Personally, I prefer 'column' over 'field' when it comes to a database table.

  • Author Developer

    I picked field, because in the rest of the existing class Drupal\Core\Database\Query\Condition it is all about fields and not about columns. If this was a greenfield solution, I would go with column, only it is not. Again: if you after reading this still think we should change something, then I will do that.

  • Please register or sign in to reply
  • 135 *
    136 * @param string $field
    137 * The name of the field to compare.
    138 * @param string $field2
    139 * The name of the other field to compare.
    140 * @param string|null $operator
    141 * (optional) The operator to use. The supported operators are: =, <>, <,
    142 * <=, >, >=, <>.
    143 *
    144 * @return $this
    145 * The called object.
    146 *
    147 * @throws \Drupal\Core\Database\InvalidQueryException
    148 * If passed invalid arguments, such as an empty array as $value.
    149 */
    150 public function compare(string $field, string $field2, ?string $operator = '=') {
    • Again, terminology..., and I would suggest to have the operator explicit, not optional, and between the two column names... so that things get much more readable.

      Suggested change
      150 public function compare(string $field, string $field2, ?string $operator = '=') {
      150 public function compareColumns(string $columnLeft, string $operator, string $columnRight) {

      then it would get to ->compareColumns("base_table.$revision_field", '<', "base_table_2.$revision_field"));

      Alternative to compareColumns() --> columnsCondition(), or even simply columns()?

      I do not think it's a big ask to developers to be clear what they want to do.

    • Author Developer

      I picked compare(), because it is essentially similar to the method condition() only with 2 fields instead of an field and a value. I picked field, because in the rest of the existing class Drupal\Core\Database\Query\Condition it is all about fields and not about columns. If this was a greenfield solution, I would go with column, only it is not. I made the operator the last parameter, because the method is mostly going to be used in join conditions and in >90% the cases the operator used is going to be an equal sign. When the operator is the last parameter and it defaults to an equal sign, the operator in >90% does not need to be set. Just like it is with the method condition(). My idea was that the new method would be the same as the method condition(), only for 2 fields instead of a field and a value. That is what Drupal developers know and expect. For me that would be the best developer experience (DX). If you after reading this still think we should change something, then I will do that.

    • daffie changed this line in version 18 of the diff

      changed this line in version 18 of the diff

    • Please register or sign in to reply
  • mondrake @mondrake started a thread on the diff
  • 142 * <=, >, >=, <>.
    143 *
    144 * @return $this
    145 * The called object.
    146 *
    147 * @throws \Drupal\Core\Database\InvalidQueryException
    148 * If passed invalid arguments, such as an empty array as $value.
    149 */
    150 public function compare(string $field, string $field2, ?string $operator = '=') {
    151 if (!in_array($operator, ['=', '<', '>', '>=', '<=', '<>'], TRUE)) {
    152 throw new InvalidQueryException(sprintf("In a query compare '%s %s %s' the operator must be one of the following: '=', '<', '>', '>=', '<=', '<>'.", $field, $operator, $field2));
    153 }
    154
    155 $this->conditions[] = [
    156 'field' => $field,
    157 'field2' => $field2,
    • Comment on lines +156 to +157

      I see here 'field' is preexisting, so agree we should not touch it but rather become 'field' => $columnLeft,

      Is there a more meaningful name for 'field2'? 'compareField', or something

    • Author Developer

      I picked field2, because in the rest of the existing class Drupal\Core\Database\Query\Condition it is all about fields and not about columns. If this was a greenfield solution, I would go with column, only it is not. Again: if you after reading this still think we should change something, then I will do that.

    • Please register or sign in to reply
  • 161 $this->changed = TRUE;
    162
    163 return $this;
    164 }
    165
    166 /**
    167 * Update the alias placeholder in the condition and its children.
    168 *
    169 * @param string $placeholder
    170 * The value of the placeholder.
    171 * @param string $alias
    172 * The value to replace the placeholder.
    173 *
    174 * @internal
    175 */
    176 public function updateAliasPlaceholder(string $placeholder, string $alias): void {
  • 631 631 * {@inheritdoc}
    632 632 */
    633 633 public function addJoin($type, $table, $alias = NULL, $condition = NULL, $arguments = []) {
    634 if (!empty($condition) && !$condition instanceof ConditionInterface) {
  • 437 438 // Only the data table follows the entity language key, dedicated field
    438 439 // tables have a hard-coded 'langcode' column.
    439 440 $langcode_key = $entity_type->getDataTable() == $table ? $entity_type->getKey('langcode') : 'langcode';
    440 $placeholder = ':langcode' . $this->sqlQuery->nextPlaceholder();
    441 $join_condition .= ' AND [%alias].[' . $langcode_key . '] = ' . $placeholder;
    442 $arguments[$placeholder] = $langcode;
    441 if ($join_condition instanceof ConditionInterface) {
    442 $join_condition->condition('%alias.' . $langcode_key, $langcode);
    443 }
    444 // Start of BC layer.
    445 else {
    446 $placeholder = ':langcode' . $this->sqlQuery->nextPlaceholder();
    447 $join_condition .= ' AND [%alias].[' . $langcode_key . '] = ' . $placeholder;
    448 $arguments[$placeholder] = $langcode;
    449 }
    450 // End of BC layer.
  • 132 132 // Run a select query to return 'last_prefix' from {simpletest_test_id} and
    133 133 // 'test_class' from {simpletest}.
    134 134 $select = $this->connection->select($max_message_id_subquery, 'st_sub');
    135 $select->join('simpletest', 'st', '[st].[message_id] = [st_sub].[max_message_id]');
    136 $select->join('simpletest_test_id', 'sttid', '[st].[test_id] = [sttid].[test_id]');
    135 $select->join('simpletest', 'st', $this->connection->condition('AND')->compare('st.message_id', 'st_sub.max_message_id'));
    136 $select->join('simpletest_test_id', 'sttid', $this->connection->condition('AND')->compare('st.test_id', 'sttid.test_id'));
    • Comment on lines +135 to +136

      Ignorance... why do we need the ->condition('AND') here?

      Edit - docs for Condition::__construct() say to use 'AND' or 'OR', but in this case we are not specifiying multiple conditions to be ANDed or ORed together, can we just skip the 'AND' argument?

    • Author Developer

      I can create a followup issue, so that the default operator for multiple conditions is AND. Personally I do not mind having to set the operator explicitly. Less change to make a mistake. I leave it up to you if we need a followup.

    • daffie changed this line in version 3 of the diff

      changed this line in version 3 of the diff

    • Please register or sign in to reply
  • mondrake @mondrake started a thread on the diff
  • 32 43 $query = $this->select('upload', 'u')
    33 44 ->distinct()
    34 45 ->fields('u', ['nid', 'vid']);
    35 $query->innerJoin('node', 'n', static::JOIN);
    46
    47 // Start of BC layer.
    48 if (is_string(static::JOIN)) {
  • Author Developer

    @mondrake: Thank you for the review.

  • daffie added 1 commit

    added 1 commit

    • b85b1f75 - Added deprecations to the methods: join(), innerJoin() and leftJoin()

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    • adffd6eb - Added the method Select::joinCondition()

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    • 409efb74 - More use of the method joinCondition()

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    • 05d648cf - More changes to the use of joinCondition()

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    • 20326dc7 - Added the method joinCondition to the selectextender

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    • 3a235aff - Fixed wrong change in Drupal\migrate\Plugin\migrate\id_map\Sql

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    • 1828aa2b - Fix the rong change to Subquery plugin

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    • 759c4a39 - Fixed change to WorkspacesAliasRepository

    Compare with previous version

  • daffie added 1 commit

    added 1 commit

    • a4a32dd5 - Removed the added deprecation of query conditions as strings and removed the...

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading