I am trying to create a relationship handler for Profile2 which limits the relationship join to certain profile types. (This is analogous to a nodequeue relationship restricting to selected queues, or a flag relationship restricting to certain flags.)

I've subclasses the main views_handler_relationship class, and it looks like in the query() method, I just need to add an 'extra' to the join definition.

This is what I'm doing:

    $def['extra'] = array(
      array(
        //'table' => $this->definition['base'],  // turns out this is not needed
        'field' => 'type',
        'operator' => 'IN', // or this, strictly speaking
        'value' => array('main', 'foo'), // for debugging I'm hardcoding these;
          // normally they would come from option values.
      ),
    );

The PDO chokes on the query though, with this error:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''main', 'foo' WHERE (( (users.status <> '0') ))) subquery' at line 1: SELECT COUNT(*) AS expression FROM (SELECT 1 AS expression FROM {users} users LEFT JOIN {profile} profile_users ON users.uid = profile_users.uid AND profile_users.type IN :views_join_condition_0_0, :views_join_condition_0_1 WHERE (( (users.status <> :db_condition_placeholder_0) ))) subquery; Array ( [:db_condition_placeholder_0] => 0 [:views_join_condition_0_0] => main [:views_join_condition_0_1] => foo ) in views_plugin_pager->execute_count_query() (line 140 of /Users/joachim/Sites/7-drupal/sites/all/modules/views/plugins/views_plugin_pager.inc).

Clearly the problem is that the "IN views_join_condition_0_0, :views_join_condition_0_1" part needs parentheses around the placeholders.

I can see where in the view_join class this should be added (patch attached fixes this in one line).

However, this fix is no good. This is the error with the patch applied:

PDOException: SQLSTATE[HY093]: Invalid parameter number: number of bound variables does not match number of tokens: SELECT COUNT(*) AS expression FROM (SELECT 1 AS expression FROM {users} users LEFT JOIN {profile} profile_users ON users.uid = profile_users.uid AND profile_users.type IN ( :views_join_condition_0 ) WHERE (( (users.status <> :db_condition_placeholder_0) ))) subquery; Array ( [:db_condition_placeholder_0] => 0 [( :views_join_condition_0 )_0] => main [( :views_join_condition_0 )_1] => foo ) in views_plugin_pager->execute_count_query() (line 140 of /Users/joachim/Sites/7-drupal/sites/all/modules/views/plugins/views_plugin_pager.inc).

What's going on here is even weirder. The placeholders are broken -- [( :views_join_condition_0 )_0]

At the very end of build_join(), the view_join class calls this:

$select_query->addJoin($this->type, $right_table, $table['alias'], $condition, $arguments);

And at this point, the condition string still has only one placeholder, like this:

users.uid = profile_users.uid AND profile_users.type IN ( :views_join_condition_0 )

So something is going wrong in $select_query, which according to the build_join() header is a SelectQueryInterface -- I'm not entirely sure what that is.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

Status: Active » Needs work
FileSize
1.11 KB

Here is a patch with a possibly incorrect fix to the first problem, which then leads on to the second problem.

joachim’s picture

Hmm.... the only declaration of addJoin in the whole of my D7 is this:

  public function addJoin($type, $table, $alias = NULL, $condition = NULL, $arguments = array()) {
    return $this->query->addJoin($type, $table, $alias, $condition, $arguments);
  }

which doesn't tell me much...

What I can say though is that the documentation for this says $arguments should be an array, whereas Views is passing in this:

'( :views_join_condition_0 )' =>
  array (
    0 => 'main',
    1 => 'foo',
  ),
)

Ignore the parentheses, that's from my patch.
Note the way it's an array of arrays. Now the query appears to be handling this by expanding the placeholder foo_0 into foo_0_0 and foo_0_1, but it's not doing so correctly, and I can't see anything in the documentation that this is an expected consequence.

joachim’s picture

Status: Needs work » Needs review
FileSize
1.68 KB

Here's a patch that works.

I figured that if the array-in-array in $arguments is undocumented, then it's probably wrong ;)

I'm not entirely happy with the variable $placeholders being plural and sometimes only holding one -- but alternatives would probably mean jumping through more hoops.

joachim’s picture

Status: Needs review » Needs work
+++ b/includes/handlers.inc
@@ -1389,22 +1389,31 @@ class views_join {
+            $placeholders = '( ' . implode(', ', array_keys($arguments)) . ' )';

Arg. That's going to break when the count == 1 passes and changed the operator from IN to = ...

Powered by Dreditor.

joachim’s picture

Status: Needs work » Needs review
FileSize
2.14 KB

One of the problems this code is hard to figure out is that while there's really only the single and the multiple case, the way they are handled it ends up being 3 cases like this:

case M
  secretly case S
  or
  really case M
or
actually case S

So let's clean that up to:

are you case S pretending to be case M? Let's fix you.
// and now:
case M
or
case S

Here's a patch that does this refactoring and fixes the original bug.

joachim’s picture

u8915055’s picture

Subscribe

dawehner’s picture


+            $info['value'] = array_shift($info['value']);

Wouldn't reset make a bit more sense logical. But sure the two functions do the same.


+              $placeholder_i = ':views_join_condition_' . $select_query->nextPlaceholder();

We could use the views placeholder function. It takes care that there is a unique amount of placeholders.
Sadly neither $view nor $query is availible in this context.

joachim’s picture

I always find reset() a bit disturbing, because its primary function is to change the array pointer (whatever that actually is!) and returning the first element is just a side-effect -- or at least that is the way the PHP documentation appears to state it.

Jerome F’s picture

Subscribing (I'm also interested in the issue described in #6)

Jerome F’s picture

The patch is still ok with views 1303951428 (28th April dev release), with it the patch in http://drupal.org/node/1114454 works well.
I you don't apply this here's the error message you get:

	•	Warning : Illegal offset type in isset or empty in views_query_views_alter() (line 1633 in (...)/Sites/acquia-drupal/sites/all/modules/views/views.module).
	•	Warning : Illegal offset type in isset or empty in views_query_views_alter() (line 1633 in (...)/sites/all/modules/views/views.module).
	•	SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ''2_espace_abonne', '0' LEFT JOIN profile profile_users_1 ON users.uid = profile_' at line 1

So I vote for RTBC - but I don't change the status as I don't understand well enough #8

DamienMcKenna’s picture

Title: join 'extra' fails with multiple value » JOIN 'extra' fails with multiple value

Have been seeing this too on a view which used a Nodequeue relationship but did not have any results, same first two errors as in #11. The patch in #8 resolved those errors and I haven't seen any other problems yet with some light testing.

Jerome F’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #5 works for me

merlinofchaos’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed.

Jerome F’s picture

Thank you

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

SanderJP’s picture

Thanks! #5 fixed it, very nice to find a quick, easy fix :) keep it up