WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#34723 closed defect (bug) (fixed)

Warning in get_the_terms() because of non-array

Reported by: mrppp Owned by: boonebgorges
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Taxonomy Keywords: fixed-major has-patch
Focuses: Cc:

Description (last modified by swissspidy)

Using Events manager plugin on events i get When updating to 4.4 on test site, all my events disappear from calendar and on listing page or when viewing the event from settings/events i get

Warning: array_map(): Argument #2 should be an array in `C:\xampp\htdocs\xxxxxxx\wp-includes\category-template.php on line 1158`

is this a WP issue or events manager?

Attachments (4)

34723.patch (1.6 KB) - added by stephenharris 3 years ago.
34723.2.patch (763 bytes) - added by joedolson 3 years ago.
Tests is_array() before mapping
34723.3.patch (1.2 KB) - added by DvanKooten 3 years ago.
Test for is_array only before calling array_map
34723.4.patch (769 bytes) - added by DvanKooten 3 years ago.
Strict type checking.

Download all attachments as: .zip

Change History (26)

#1 @swissspidy
3 years ago

  • Component changed from General to Taxonomy
  • Description modified (diff)
  • Summary changed from WP 4.4 on test site to Warning in get_the_terms() because of non-array

Hard to tell. Have you informed the developers of that plugin? They should test it against 4.4, especially when they would be using term meta stuff.

That line belongs to get_the_terms(). At first glance it looks like either the return value of get_object_term_cache() or wp_get_object_terms() isn't properly checked.

Especially wp_get_object_terms() can return a WP_Error when the taxonomy does not exist. wp_get_object_terms() should account for that scenario.

#2 @mrppp
3 years ago

Hi, reported to plugin developer, thanks

@stephenharris
3 years ago

#3 @stephenharris
3 years ago

  • Keywords has-patch has-unit-tests added
  • Version set to 4.4

I believe this is a bug in WordPress, specifically in get_the_terms(). See this line: https://github.com/WordPress/WordPress/blob/77e365efbf2e499e2ed11d29c101ea466cf1ceed/wp-includes/category-template.php#L1158 and the foreach loop a few lines above it - both assume that $terms is an array, but wp_get_object_terms() will return a WP_Error when the taxonomy doesn't exist.

In 4.3.1 get_the_terms() could be used with a non-registered taxonomy without any issues (it would return a WP_Error()). Currently array_map on WP_Error object would give the reported error.

Looks like this issues was introduced in [35032]

#4 @johnbillion
3 years ago

  • Milestone changed from Awaiting Review to 4.4.1

#5 @boonebgorges
3 years ago

The array_map() error is new (introduced in [35032], as noted by @stephenharris), but as @swissspidy notes, it appears to be an old bug that get_the_terms() can accidentally cache a WP_Error object. Whoops.

#6 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 35850:

Improve handling for WP_Error objects in get_the_terms().

wp_get_object_terms() can return a WP_Error object. As such, the
get_the_terms() cache wrapper should handle them properly. To wit:

  • Don't try to map an error object to get_term(). Introduced in [35032].
  • Don't cache an error object as taxonomy relationships. Introduced in at least [16487], maybe earlier.

Props stephenharris.
Fixes #34723.

#7 @boonebgorges
3 years ago

  • Keywords fixed-major added; has-patch has-unit-tests removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.4.1

#8 @boonebgorges
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 35851:

Improve handling for WP_Error objects in get_the_terms().

wp_get_object_terms() can return a WP_Error object. As such, the
get_the_terms() cache wrapper should handle them properly. To wit:

  • Don't try to map an error object to get_term(). Introduced in [35032].
  • Don't cache an error object as taxonomy relationships. Introduced in at least [16487], maybe earlier.

Ports [35850] to the 4.4 branch.

Props stephenharris.
Fixes #34723.

#9 @joedolson
3 years ago

  • Keywords has-patch added
  • Resolution fixed deleted
  • Status changed from closed to reopened

get_object_term_cache() is used to set $terms before calling wp_get_object_terms(), and that function returns either an array or boolean false. If it returns false, it will return true for ! is_wp_error() and throw a warning when a non-array is passed to array_map.

@joedolson
3 years ago

Tests is_array() before mapping

#10 @stephenharris
3 years ago

It doesn't hurt to check, though I don't think this should happen: If get_object_term_cache() returns false then $terms is replaced by the value returned by wp_get_object_terms() (an array or WP_Error).

So get_object_term_cache() would have to return true or otherwise one of those functions would have to return a undocumented value type.

#11 @joedolson
3 years ago

Hmm. Yes, you're right. I did get this triggered; but I'll have to look at the test case more carefully and see why it returned that.

#12 @boonebgorges
3 years ago

What @stephenharris says in comment 10 looks correct to me. By the time we get to the ! is_wp_error( $terms ) check, $terms should be set to the return value of wp_get_object_terms(); this latter function can only return an array or a WP_Error object.

@joedolson It's possible that you'd get an error like this if the {$taxonomy}_relationships cache contains a non-array value. Are you sure that your array_map() error is being caused by $terms being set to false? It seems more likely that it'd be caused by a plugin (or core) caching a scalar value (or a WP_Error object).

On a potentially related note, it appears that there's a related bug in two other places in core:

As in the case of get_the_terms(), it's possible that a WP_Error object could be erroneously cached here. This is not a regression for 4.4 (and [35851] should cover any potential collateral damage when get_the_terms() attempts to access a corrupt cache), but probably ought to be addressed. @joedolson Is it possible that your issue stems from one of the above, or a similar issue in a plugin?

#13 @joedolson
3 years ago

That *is* possible; I'll have to investigate further tomorrow.

#14 @joedolson
3 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Yes, it was an invalid value stored in cache. This was definitely because of a bug in a plug-in, so I don't think there's any concern for core here. 35851 does cover the issue. Thanks!

#15 @tivnet
3 years ago

#35265 was marked as a duplicate.

#16 @boonebgorges
3 years ago

#35265 was marked as a duplicate.

#17 @DvanKooten
3 years ago

Doesn't it make more sense to just check for an array type before calling array_map (instead of checking for WP_Error)?

Theoretically, it will always be an array or WP_Error at that point but we should be coding for the future, right? :)

@DvanKooten
3 years ago

Test for is_array only before calling array_map

#18 follow-up: @DvanKooten
3 years ago

Oops. I messed up that previous patch - how do I remove it?

After looking at this a bit more, it seems to make sense to perform stricter checking in the other places as well. We really only want to update the cache & map each term if we got an array so the attached patch should make more sense.

The code can be a lot simpler if the WP_Error object doesn't need to run through the get_the_terms filter. Any reason you want to do this @swisspidy?

@DvanKooten
3 years ago

Strict type checking.

#19 in reply to: ↑ 18 @swissspidy
3 years ago

Replying to DvanKooten:

Oops. I messed up that previous patch - how do I remove it?

After looking at this a bit more, it seems to make sense to perform stricter checking in the other places as well. We really only want to update the cache & map each term if we got an array so the attached patch should make more sense.

The code can be a lot simpler if the WP_Error object doesn't need to run through the get_the_terms filter. Any reason you want to do this @swisspidy?

Me (with three s's btw)? Good question :-)

I assume this was done so plugins can hook into get_the_terms to return their own terms from somewhere, even when it's a WP_Error object. Removing this would break backwards compatibility.

Besides that, 34723.4.patch makes sense to me.

#20 @boonebgorges
3 years ago

@DvanKooten Thanks for the patch. As it stands, it won't quite work - an empty array is a valid cache entry, and shouldn't count as a miss. It's standard practice throughout WP to do a strict false === comparison when checking for cached values, for this and similar reasons.

That said, it's possible that we could make the logic here more future-forward :) If you want to propose another approach, can you please open a different ticket that references this one (which is against a closed milestone)?

#21 follow-up: @DvanKooten
3 years ago

Hi @boonebgorges ,

But an empty array would pass the is_array check and thus be cached, unless I am missing something here?

#22 in reply to: ↑ 21 @boonebgorges
3 years ago

Replying to DvanKooten:

Hi @boonebgorges ,

But an empty array would pass the is_array check and thus be cached, unless I am missing something here?

Oh right, sorry, long day.

Note: See TracTickets for help on using tickets.