Warning in get_the_terms() because of non-array

[mrppp]

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?

[swissspidy]

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.

[mrppp]

Hi, reported to plugin developer, thanks

[stephenharris]

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]

[boonebgorges]

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.

[boonebgorges]

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.

[boonebgorges]

Reopening for 4.4.1

[boonebgorges]

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.

[joedolson]

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]

Tests is_array() before mapping

[stephenharris]

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.

[joedolson]

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.

[boonebgorges]

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:

• https://core.trac.wordpress.org/browser/tags/4.4/src/wp-admin/includes/template.php?marks=311-315#L305
• https://core.trac.wordpress.org/browser/tags/4.4/src/wp-admin/includes/taxonomy.php?marks=242-245#L228

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?

[joedolson]

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

[joedolson]

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!

[tivnet]

#35265 was marked as a duplicate.

[boonebgorges]

#35265 was marked as a duplicate.

[DvanKooten]

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]

Test for is_array only before calling array_map

[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?

[DvanKooten]

Strict type checking.

[swissspidy]

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.

[boonebgorges]

@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)?

[DvanKooten]

Hi @boonebgorges ,

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

[boonebgorges]

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.