WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#26728 closed defect (bug) (fixed)

Previous fix for get_queried_object, gives notices for tax queries

Reported by: layotte Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.9
Component: Query Keywords: has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Seeing this notice when browsing to a taxonomy archive page (several times):
'Notice: Undefined variable: query in /home/lew/public_html/wp-includes/query.php on line 3274'

Changed in my patch to $this->is_tax, since $query isn't defined anywhere beforehand.

related [26864], #26634, #26627.

Attachments (3)

query.php.diff (659 bytes) - added by layotte 7 years ago.
26728.diff (1.0 KB) - added by SergeyBiryukov 7 years ago.
26728.test.diff (946 bytes) - added by SergeyBiryukov 7 years ago.

Download all attachments as: .zip

Change History (22)

@layotte
7 years ago

#1 @Otto42
7 years ago

  • Keywords has-patch added

Makes sense. $query is the wrong thing to use there.

#2 @lkraav
7 years ago

  • Cc leho@… added

#3 @SergeyBiryukov
7 years ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 3.8.1

#4 @Chouby
7 years ago

  • Cc frederic.demarle@… added

I believe that at this point $this->is_tax is always true. So the proposed patch should reintroduce the bug corrected in [26864]. I thus propose to test for:

empty($query['terms'])

instead of only

$query['terms']

That would be strange enough if this notice was not present in WP < 3.8 since [26864] reintroduced the test exactly as it was before [26007].

#5 @layotte
7 years ago

If $this->is_tax is always true, than the line 3265 above will always be true:

 if ( $this->is_category || $this->is_tag || $this->is_tax )

And $query is never defined in this function, so it'll always be empty.

@SergeyBiryukov
7 years ago

#6 @Otto42
7 years ago

Seems like this code had a bug to begin with then, as $query is undefined in this function, and $query[ 'terms' ] will never have any content.

Rather than reintroducing a bug, let's determine what the code should be doing here. I cannot see why is_tax would always be true here, except insofar as the check before it was only checking for those cases.

Last edited 7 years ago by Otto42 (previous) (diff)

#7 @Otto42
7 years ago

26728.diff looks good, but I don't think the check for if ( $query[ 'terms' ] ) is necessary. The field, terms, and taxonomy should all be set at that point from the $tax_query_in_and.

#8 @layotte
7 years ago

That's almost what was in [26007]... then in [26864] the elseif ( $query['terms'] ) was added. Perhaps wonderboymusic can shed some light (I don't know if it's possible to tag someone in a trac post).

Last edited 7 years ago by SergeyBiryukov (previous) (diff)

#9 @layotte
7 years ago

For what it's worth, $this->is_tax is not always true at this point (from my quick testing).

#10 follow-ups: @SergeyBiryukov
7 years ago

Seems like 26728.diff would be the correct fix there.

Replying to Chouby:

That would be strange enough if this notice was not present in WP < 3.8 since [26864] reintroduced the test exactly as it was before [26007].

Not exactly, $query was defined at that point before [26007] (see line 3253 on the left).

Replying to layotte:

If $this->is_tax is always true, than the line 3265 above will always be true:

No, it's vice versa. If we're inside that condition, then either $this->is_category, $this->is_tag, or $this->is_tax is true. We check for $this->is_category and $this->is_tag above, so if they are both false, that means $this->is_tax is true.

Replying to Otto42:

26728.diff looks good, but I don't think the check for if ( $query[ 'terms' ] ) is necessary.

Related: [22450], #26634.

Last edited 7 years ago by SergeyBiryukov (previous) (diff)

#11 in reply to: ↑ 10 @layotte
7 years ago

Replying to SergeyBiryukov:

Replying to layotte:

If $this->is_tax is always true, than the line 3265 above will always be true:

No, it's vice versa. If we're inside that condition, then either $this->is_category, $this->is_tag, or $this->is_tax is true. We check for $this->is_category and $this->is_tag above, so if they are both false, that means $this->is_tax is true.

Ah, yes, misunderstood what he meant.

+1 to 26728.diff

#12 @wonderboymusic
7 years ago

In 26874:

In WP_Query::get_queried_object(), move the check for $query['terms'] to a place where $query is actually set. This should be included if [26864] makes it into 3.8.1.

Props SergeyBiryukov.
See #26728, [26864], #26634, #26627.

This ticket was mentioned in IRC in #wordpress-dev by SergeyBiryukov. View the logs.


7 years ago

#14 @SergeyBiryukov
7 years ago

  • Keywords fixed-major added

#15 @SergeyBiryukov
7 years ago

26728.test.diff fails before [26874] and passes after it.

#16 @SergeyBiryukov
7 years ago

In 26875:

Add unit test for [26874]. see #26728.

#17 in reply to: ↑ 10 ; follow-up: @Chouby
7 years ago

Replying to SergeyBiryukov:

Not exactly, $query was defined at that point before [26007] (see line 3253 on the left).

This seems to be what we all missed in previous attempts to solve #26634

Should we close #26634 and mark it as duplicate in favor of this ticket as now the right fix is dealt here?

#18 in reply to: ↑ 17 @SergeyBiryukov
7 years ago

  • Keywords fixed-major removed
  • Milestone changed from 3.8.1 to 3.9

Replying to Chouby:

Should we close #26634 and mark it as duplicate in favor of this ticket as now the right fix is dealt here?

I think we can close this one as fixed in 3.9, and keep both #26627 and #26634 open (as they are focused on different issues) for merging the changes to 3.8.1.

#19 @SergeyBiryukov
7 years ago

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.