Notice: Undefined index: in views_query->add_table() [#887768]

This error has popped up since I upgrade to Drupal 6.18. I believe this has to do with the new error reporting capabilities.

The exact error I am getting is: Notice: Undefined index: in views_query->add_table() (line 257 of D:\sites\testsite\sites\all\modules\views\includes\query.inc).

I have had a look at the issue and looking at the code the problem is on the lines that has:

if (!$join) {
$join = $this->get_join_data($table, $this->relationships[$relationship]['base']);
}

The notice occurs when $relationship and $join are both set to NULL. This is allowing the above statement to be fired and as $relationship is null and array undefined index is occuring. To fix I am proposing checking the value is set using isset as follows.

if (!$join && isset($this->relationships[$relationship]['base'])) {
$join = $this->get_join_data($table, $this->relationships[$relationship]['base']);
}

This seems to have made the errors go away without breaking anything. I have also uploaded a patch with this change.

If someone could check all this for me I would be very appreciative.

Regards,
Tim

Comment	File	Size	Author
#11	887768-fix-notice-with-joins-d6.x-2.x.patch	1.08 KB	merlinofchaos
#11
#11	887768-fix-notice-with-joins-d6.x-3.x.patch	950 bytes	merlinofchaos
#11
#9	query-inc-887768-4.patch	806 bytes	timhilliard
#9
#4	query_inc.patch	588 bytes	timhilliard
#4
	query.inc_.patch	388 bytes	timhilliard

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

timhilliard CreditAttribution: timhilliard commented 19 August 2010 at 10:59

I should also have added that the problem seems to be occuring when creating a view that aggregates 3 different content types and uses unique fields to each content type in the display fields. In this case the fields that were causing the problem were imagefield using imagecache as the display.

Comment #2

merlinofchaos CreditAttribution: merlinofchaos commented 9 December 2010 at 00:39

Assigned:	Unassigned	» dawehner
Priority:	Normal	» Critical
Status:	Needs review	» Needs work

There's a couple of problems here, and I'm confused about this code.

1) The most common use of $add_table() is to add a table with no relationship or join. That's the case that's going to fail here, because the get_join_data will fail.
2) This code doesn't exist in Views 3.x for either D6 or D7.
3) The patch still isn't right because it doesn't avoid the adjust_join that shouldn't happen if there isn't a join.
4) Looking at this, I don't know why anything works. :P

dereine, I'm passing this to you, because this is a case where we can benefit from some tests to get it right. We need to test add_table in probably a dozen different cases, both with and without relationships, with and without custom joins, etc, and make sure that the query it creates is correct at the end. From those tests we should be able to figure out the proper way to fix this.

Ordinarily I don't mark non-crash bugs critical, but this feels like it SHOULD be a crash bug. I'm not sure why it's not, but it's also really fundamental to Views' query builder and I'm shocked that after 3 years this isn't right.

Comment #3

marcvangend

he/him

Dutch

Amsterdam

CreditAttribution: marcvangend commented 13 December 2010 at 11:00

subscribe, seeing the same error.

Comment #4

timhilliard CreditAttribution: timhilliard commented 15 December 2010 at 15:53

Status:

Needs work

» Needs review

File	Size
query_inc.patch	588 bytes

Hi guys, would the patch needed be as simple as the one I attached? This should now avoid the adjust_join that you mentioned. I think the reason it didn't fail however was that adjust_join returns $join which in the error case would have been NULL when $relationship is empty. Which is essentially setting itself to NULL although it's already NULL. This should then let us add the table whilst $relationship and $join are both NULL.

The error being returned was only a PHP Warning about array keys so I don't think this is a big thing but still needs to be fixed none the less.

function add_table($table, $relationship = NULL, $join = NULL, $alias = NULL) {
    if (!$this->ensure_path($table, $relationship, $join)) {
      return FALSE;
    }

    if (!$join && isset($this->relationships[$relationship]['base'])) {
      $join = $this->get_join_data($table, $this->relationships[$relationship]['base']);
    }

    if($join) $join = $this->adjust_join($join, $relationship);

    return $this->queue_table($table, $relationship, $join, $alias);
}

Cheers,
Tim

Comment #5

timhilliard CreditAttribution: timhilliard commented 15 December 2010 at 15:57

An alternative to the second if statement would be

if (!$join && $relationship) {

as having $relationship set to NULL was the problem and not so much that the array key didn't exist. Not sure which one would be better however.

Comment #6

merlinofchaos CreditAttribution: merlinofchaos commented 15 December 2010 at 18:36

I think it might be that simple. It's worth noting that the patch does not conform to coding style -- in particular all ifs must have braces and be properly formatted. (I find this to be a critical style issue for readability. I am highly trained to a particular form of statement)

I think we need tests to prove it, though.

Comment #7

NROTC_Webmaster CreditAttribution: NROTC_Webmaster commented 3 January 2011 at 01:49

timhilliard,

I added your code to my file and it seems to be working. I will updated if I see this error again but it only happens every once in a while so it is hard to definitively confirm it is a fix.

Comment #8

dawehner

German

CreditAttribution: dawehner commented 3 January 2011 at 10:52

Add tag

Comment #9

timhilliard CreditAttribution: timhilliard commented 5 January 2011 at 02:17

File	Size
query-inc-887768-4.patch	806 bytes

Re-rolled patch with correct drupal styling however I'm still learning how to write tests so it may well be much faster if someone else can do that.

Cheers,
Tim

Comment #10

joshuajabbour CreditAttribution: joshuajabbour commented 27 January 2011 at 19:17

Status:

Needs review

» Reviewed & tested by the community

patch in #9 definitely suppressed the notice for me. I would get it always when visiting a particular view page, and with this patch, it's gone (and the view works fine)…

Comment #11

merlinofchaos CreditAttribution: merlinofchaos commented 4 February 2011 at 23:16

Priority:	Critical	» Normal
Status:	Reviewed & tested by the community	» Needs review

File	Size
887768-fix-notice-with-joins-d6.x-3.x.patch	950 bytes

887768-fix-notice-with-joins-d6.x-2.x.patch	1.08 KB

Ok, I spent a little time tracing through to try and figure out what was going on. The fact that this code isn't matching between 2.x and 3.x was bugging me, and it really should match. The $join fetch isn't actually necessary there, I think, and the adjust join only needs to happen if there's already a join. So let's try this set of patches which makes both systems work.

I'd appreciate it if people with chained nodereference relationships or something equally complicated could test this out for me:

I just noticed this is marked critical, but it's only a notice error. Notice errors are not critical, by any means.

Comment #12

joshuajabbour CreditAttribution: joshuajabbour commented 8 February 2011 at 03:42

2.x patch is RTBC...

Comment #13

Stalski CreditAttribution: Stalski commented 15 February 2011 at 20:14

2.x patch seems to do the job

Comment #14

neclimdul

He/Him

CreditAttribution: neclimdul commented 29 April 2011 at 15:52

Seems fine to me. Fixes a "Missing join" vpr warning I was seeing on views with Date fields because of this code:

  function add_date_field($field) {
    // Explicitly add this table using add_table so Views does not
    // remove it if it is a duplicate, since that will break the query.
    $this->query->add_table($field['table_name'], NULL, NULL, $field['table_name']);
    return;
  }

Since ensure_path will know how to handle that and we don't care about the return the patch seems to correctly handle the call.

I've only tested it in 2.x as well so we still need a 3.x review.