51423: wp-includes/bookmark.php PHP 8 compatibility fix for array_unique

[hellofromtonya]

Trac ticket: https://core.trac.wordpress.org/ticket/51423

Fixes array_unique in wp-includes/bookmark.php:

"Parameter 1 $input of function array_unique expects array, array|WP_Error given."

• wp_get_object_terms returns array or WP_Error. Check if an error is returned. If no, then pass the array into array_unique; else, return the error.

• Adds happy and unhappy path tests for get_bookmark() function.

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[jrfnl]

Hi @hellofromtonya

Thanks for the ping. Very happy to see the first of the broken out fixes with extensive unit tests to boot!!

I've had a good look at this and the current proposal constitutes a behavioural change in the function output, which, as wp_cache_add() is used, also ripples through into the cache and is undocumented.

If we look at the original behaviour of the line of code in question - see https://3v4l.org/vUnPH -, we can see that $_bookmark->link_category would previously either be: an array, which could be filled or empty, or null if wp_get_object_terms() would have returned WP_Error.

With the proposed change, this will change to: an array or a WP_Error object, which is unexpected and makes handling the error condition for plugins/themes supporting multiple WP versions more complicated, as they now have to deal with the possibility of an empty array, null or WP_Error.

Previously, if ( ! empty( $bookmark->link_category ) ) {} in a plugin/theme using the return of the function would have worked, now it no longer will as WP_Error will also need to be accounted for.

I'd like to suggest considering either one of the following two alternatives:

• Either maintain the previous behaviour:

  $_bookmark->link_category = ! is_wp_error( $link_category ) ? array_unique( $link_category ) : null;

• Or make the function return value more consistent by ensuring that $_bookmark->link_category will always be an array, either empty or filled:

  $_bookmark->link_category = ! is_wp_error( $link_category ) ? array_unique( $link_category ) : array();

If the second option would be chosen, I'd recommend adding a @since 5.6.0 ... entry detailing that the link_category property/index key of the return value will now always be an array, where it could previously also have been null.

Now, there is still the fact that there was an error to deal with.

Previously the function would have thrown a native PHP warning: Warning: array_unique() expects parameter 1 to be array, object given, which is semi-obscure, but would at least have given an indication to the plugin/theme/core developer that an error condition was hit.

The current fix removes that warning, leaving no indication that there was an error (other than the unexpected WP_Error object).

We could consider throwing a warning from within the function instead. This, again, would maintain the previous behaviour more closely and it could also provide useful information to the developer.

If it would be elected to do so, the code would need to change to something along the lines of:

-$_bookmark->link_category = ! is_wp_error( $link_category ) ? array_unique( $link_category ) : $link_category;
+if ( ! is_wp_error( $link_category ) ) {
+    $_bookmark->link_category = array_unique( $link_category );
+} else {
+    trigger_error( $link_category->get_error_message(), E_USER_WARNING);
+    $_bookmark->link_category = $link_category; // TBD what this should become (see above).
+}

The error message recorded in WP_Error will need to be verified for its usefulness. If it is not that informative, we could consider a custom error message along the lines of "The 'link_category' taxonomy does not exist".

What do you think ?

Regarding the tests, I wonder if the data provider data_bookmark and the helper functions could be simplified a little.
The first and the second parameter are always linked and that link should be maintained, while the use of the callback via $this->$get_expected() obscures that.

Suggestion:

• Remove the first parameter from the data provider.

  array( OBJECT, 'display' ),
  array( ARRAY_A, 'raw' ),

• And have just one get_expected() helper method which expects the $output parameter to be passed:

  protected function get_expected( $output ) {
      switch ( $output ) {
          case ARRAY_A:
              return ...
          case ARRAY_N:
              return ...
          default:
              return ...
      }
  }

Other than that, I haven't studied the tests in detail as I think the functional change above needs to be discussed first.

Refs:

• https://developer.wordpress.org/reference/functions/wp_get_object_terms/
• https://developer.wordpress.org/reference/functions/wp_cache_add/

[hellofromtonya]

@jrfnl You are correct.

Changed code's behavior

The code's behavior was changed when WP_Error is returned when 'link_category' taxonomy is not registered. This is not desired as it could be a breaking change with no benefits.

This was bugging me last night, but wasn't well-formed. Thank you for thoroughly and clearly laying out why and next steps. 👏

Updates:

• Commit db3e545 returns the original behavior by setting link_category property to null when the returned value is not an array. A few notes:

  • is_wp_error() fires an action. Using it then also changes the behavior.
  • Switched to check for the non-error state, i.e. array where wp_get_object_terms() returns array unless the taxonomy does not exist, then returns WP_Error.

• Commit b3eb1d7 triggers the warning to retain the behavior and be a clue and indication if something went wrong.

Test Cases

• Commit 96f915d: Took your suggestion to simplifying getting the expected value. Good idea 💡 .

• Commit ad73fe0: Doh on my part.

[jrfnl]

@hellofromtonya Glad you found the feedback useful.

As a side-note: the chances of this error condition ever being hit in a real life situation are very small as that would involve a plugin/theme explicitly (and potentially maliciously) de-registering the WP native link_category taxonomy and if that would happen, more things are likely to break anyhow.

Having said that, this doesn't negate the validity of this patch. No matter how small the chance of an error condition being hit, the error condition should preferably be handled gracefully, which is now is. So 👍 for me for this patch to be committed.

[hellofromtonya]

Moved creating the test class for get_bookmark() to PR #1180.