Make WordPress Core

Opened 10 years ago

Closed 10 years ago

#27489 closed defect (bug) (worksforme)

If non-string passed to taxonomy_exists() php throws warning

Reported by: jackreichert's profile jackreichert Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.0
Component: Taxonomy Keywords: has-patch
Focuses: Cc:

Description

To reproduce the bug pass the object returned from get_object_taxonomies() to get_terms().

Result, received the following error:

[Sat Mar 22 08:21:10 2014] [error] [client 192.168.159.1] PHP Warning:  Illegal offset type in isset or empty in /var/www/html/wordpress/wp-includes/taxonomy.php on line 226, referer: http://localhost/wp-admin/about.php

Attachments (2)

taxonomy.php.patch (655 bytes) - added by jackreichert 10 years ago.
patch to fix bug
taxonomy._doing_it_wrong.diff (688 bytes) - added by jackreichert 10 years ago.
doing it wrong implementation

Download all attachments as: .zip

Change History (11)

@jackreichert
10 years ago

patch to fix bug

#1 @jackreichert
10 years ago

  • Keywords has-patch added

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


10 years ago

#3 @SergeyBiryukov
10 years ago

  • Version changed from trunk to 3.0

get_terms() description says that it only accepts a string or an array of strings as its first argument:
tags/3.8.1/src/wp-includes/taxonomy.php#L1252

This looks similar to #17299, #18927, and #23767. The consensus was that we shouldn't hide warnings caused by developer errors (unless there's a strong reason). It would make debugging harder.

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

#4 @jackreichert
10 years ago

Thanks Sergey. And thanks for checking this ticket out so quickly.

I came across it and the result seemed incongruous to results from other functions. Sorry I missed those tickets, I promise that I searched before posting :)

#5 @SergeyBiryukov
10 years ago

No worries, those tickets are unrelated to taxonomy_exists(), but they offer similar enhancements for other functions.

If a valid parameter generates a warning, we should certainly fix that. However, in case of an invalid parameter, I think the warning is to be expected.

#6 @jackreichert
10 years ago

A bigger scope project might be to add custom error_log() messages to assist the debugging for functions that throw warnings like these.

#7 @SergeyBiryukov
10 years ago

We could probably add a _doing_it_wrong() call: tags/3.8.1/src/wp-includes/functions.php#L3018.

#8 @jackreichert
10 years ago

That's a great tool. Learn something new every day.
So it would be something like this? (see new uploaded patch)

@jackreichert
10 years ago

doing it wrong implementation

#9 @wonderboymusic
10 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Warnings are the best way to alert the developer that their PHP is blowing up - I don't think this function is ambiguous with what input it wants to accept. _doing_it_wrong() could possibly be used in more places, but sparingly

Note: See TracTickets for help on using tickets.