The problem is two fold and while the first part could be fixed in a separate patch it has the potential to expose more users to the second bug so I have included them both in the same patch.

As it stands now ensure_table():

  • unnecessarily special cases joins to the base table
  • considers identical joins apart from the join type (INNER or LEFT) to be different which causes undesired cross-products

base table special case

// If the relationship is the primary table, this actually be a relationship
// link back from an alias. We store all aliases along with the primary table
// to detect this state, because eventually it'll hit a table we already
// have and that's when we want to stop.
if ($relationship == $this->base_table && !empty($this->tables[$relationship][$table])) {
  return $this->tables[$relationship][$table]['alias'];
}

When there is an entire loop below to handle detection of duplicate joins. The difference being the code below additionally checks the ON clause while the special case does not. That over simplification causes multiple joins between the same two tables with unique ON clauses to be ignored when including the base table.

A hypothetical example:

base table: node n
JOIN user u1 ON u1.uid = n.uid1
JOIN user u2 ON u2.uid = n.uid2  # <-- this join is ignored when using ensure_table()

Removing the block of code above solves corrects the above example so that the second join is properly added.

merging different JOIN types

Once the above special case is removed the code will fall back to the proper (once fixed) check. Currently the code stands:

foreach ($this->table_queue as $queued_table) {
  // In PHP 4 and 5, the == operation returns TRUE for two objects
  // if they are instances of the same class and have the same
  // attributes and values.
  if ($queued_table['relationship'] == $relationship && $queued_table['join'] == $join) {
    return $queued_table['alias'];
  }
}

The problem, which relation module hits, is that two identical joins with different JOIN types are not considered the same table relationship.

Relation module's current implementation exposes relationships that represent two table joins in a single views relationship. In the original views integration by solotandem two relationships exposed (one for each table join. The relationship maintainers tried to simplify this by making a single relationships that encompassed both table joins, but this introduced some issues. Before going to much further here is an example that helps make this clear.

base table: node
relation 1 [not required]: node -> field_data_endpoints -> node
relation 2 [required]:     node -> field_data_endpoints -> relation (to which field_data_endpoints represents)

Note the double table joins represented as single relationships. This example can hit both the issues being discussed. With the special casing on the base table the second join is completely ignored (fine in this example, but not in others) which hides the lack of JOIN type checking. Once the special case is removed the JOIN is added twice since the two joins to field_data_endpoints had different types due to the differing relationship required property. This causes a cross-product since the query contains the following (simplified example).

# two separate join chains
INNER JOIN field_data_endpoints f1 ON f1.rid = n.rid
LEFT JOIN field_data_endpoints f2 ON f2.rid = n.rid

INNER JOIN node n1 ON n1.nid = f1.nid
LEFT JOIN relation r1 ON r1.rid = f2.rid

In this case the desired output would be:

# both secondary joins use the same endpoints join
INNER JOIN field_data_endpoints f1 ON f1.rid = n.rid

INNER JOIN node n1 ON n1.nid = f1.nid
LEFT JOIN relation r1 ON r1.rid = f1.rid

When two joins that are identical apart from the type INNER should always be used.

Obviously all this goes away if using the original implementation since the join to field_data_endpoints would be represented as a relationship in views and one could not add it twice and therefore it would have to be either required or not.

Since ensure_table() represents an API used by contrib it should be fixed even if views does not present a scenario like this by default (of which I am aware).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

boombatower created an issue. See original summary.

boombatower’s picture

boombatower’s picture

boombatower’s picture

As suspected, I confirmed the exact same code in Drupal 8. I can cross-post a patch, but I have no interested in testing in 8.0.x at the moment.

http://cgit.drupalcode.org/drupal/tree/core/modules/views/src/Plugin/vie...

Filed separate issue #2617062: ensureTable() does not allow two distinct joins against base table and another, while also adding duplicate relations with different join types.

Chris Matthews’s picture

Assigned: boombatower » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll

The 3 year old patch in #2 to views_plugin_query_default.inc does not apply to the latest views 7.x-3.x-dev and if still relevant needs to be rerolled.

Checking patch plugins/views_plugin_query_default.inc...
error: while searching for:
      $relationship = $this->base_table;
    }

    // If the relationship is the primary table, this actually be a relationship
    // link back from an alias. We store all aliases along with the primary table
    // to detect this state, because eventually it'll hit a table we already
    // have and that's when we want to stop.
    if ($relationship == $this->base_table && !empty($this->tables[$relationship][$table])) {
      return $this->tables[$relationship][$table]['alias'];
    }

    if (!array_key_exists($relationship, $this->relationships)) {
      return FALSE;
    }

error: patch failed: plugins/views_plugin_query_default.inc:546
error: plugins/views_plugin_query_default.inc: patch does not apply
Snehal Brahmbhatt’s picture

Status: Needs work » Needs review
FileSize
2.97 KB

@Chris Matthews, Please find my patch with the updated version, Hope this will helps you!..

Thanks!..

DamienMcKenna’s picture

Issue tags: -Needs reroll
solotandem’s picture

The first and only patch in #2 still applies to 7.x-3.x. The attachment in #6 changed nothing (the chunk line numbers are not critical and do not constitute a change). Why do you tag this for reroll? It has been RTBC for 4 years, used in production, and without it errors are present.

My apology, can't read the strikethrough this morning.