Make WordPress Core

Opened 3 years ago

Closed 18 months ago

Last modified 18 months ago

#51986 closed defect (bug) (fixed)

PHP Warning: array_intersect_key(): Expected parameter 1 to be an array, string given in class-wp-rest-server.php

Reported by: slaffik's profile slaFFik Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: normal
Severity: normal Version: 5.6
Component: Editor Keywords: php8 has-patch has-unit-tests
Focuses: rest-api Cc:

Description (last modified by desrosj)

When editing a page in the Gutenberg editor (that is built-in, not the plugin), I see this Warning on a page:

<b>Warning</b>:  array_intersect_key(): Expected parameter 1 to be an array, string given in <b>/home/cosydale/public_html/ovirium.com/wp-includes/rest-api/class-wp-rest-server.php</b> on line <b>1402</b><br />

Disable REST plugin is not installed.

Attachments (1)

51986.test.diff (736 bytes) - added by nateallen 21 months ago.
Unit test confirming fix for bug

Download all attachments as: .zip

Change History (48)

#1 @desrosj
3 years ago

  • Component changed from REST API to Editor
  • Description modified (diff)
  • Focuses rest-api added
  • Keywords reporter-feedback added

Hi @slaFFik!

Do you have any plugins registering custom REST API routes? Any plugins that are registering custom block types?

#2 @apermo
3 years ago

I have the same issue.

I've already posted some information here: https://wordpress.org/support/topic/wordpress-5-6-upgrading-issue/#post-13765662

I have the same issue, among my active plugins is redux, and quite a long list of custom plugins.

Warning: array_intersect_key(): Expected parameter 1 to be an array, string given in /wp-includes/rest-api/class-wp-rest-server.php on line 1402

Call Stack
# Time Memory Function Location
1 0.0109 488824 {main}( ) .../post.php:0
2 3.6107 69597144 require( '/wp-admin/edit-form-blocks.php' ) .../post.php:187
3 3.6108 69600920 array_reduce ( ) .../edit-form-blocks.php:82
4 3.6108 69600920 rest_preload_api_request( ) .../edit-form-blocks.php:82
5 3.6108 69601520 rest_do_request( ) .../rest-api.php:2520
6 3.6820 71674264 WP_REST_Server-&gt;dispatch( ) .../rest-api.php:479
7 3.6858 71732896 WP_REST_Server-&gt;respond_to_request( ) .../class-wp-rest-server.php:1007
8 3.6858 71732896 WP_REST_Server-&gt;get_index( ) .../class-wp-rest-server.php:1160
9 3.6900 72029472 WP_REST_Server-&gt;get_data_for_routes( ) .../class-wp-rest-server.php:1243
10 3.6909 72042136 WP_REST_Server-&gt;get_data_for_route( ) .../class-wp-rest-server.php:1324
11 3.7373 72049584 array_intersect_key ( ) .../class-wp-rest-server.php:1402

For testing I edited the code of class-wp-rest-server.php to:

foreach ( $callbackargs? as $key => $opts ) {

if ( ! is_array( $opts ) ) {

print_r( $callbackargs? );
echo "\n";
print_r( $opts );
echo "\n\n";

}
$arg_data = @array_intersect_key( $opts, $allowed_schema_keywords );
$arg_datarequired? = ! empty( $optsrequired? );

$endpoint_dataargs?[ $key ] = $arg_data;

}

Giving me debug information which lead me to redux-framework/redux-templates/classes.class-api.php and the class method register_api_hooks in line 785 of that file where all of these are registered.

Version 0, edited 3 years ago by apermo (next)

#3 @apermo
3 years ago

Index: public_html/wp-includes/rest-api/class-wp-rest-server.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/public_html/wp-includes/rest-api/class-wp-rest-server.php b/public_html/wp-includes/rest-api/class-wp-rest-server.php
--- a/public_html/wp-includes/rest-api/class-wp-rest-server.php	(date 1607525283294)
+++ b/public_html/wp-includes/rest-api/class-wp-rest-server.php	(date 1607525283294)
@@ -1399,7 +1399,7 @@
 				$endpoint_data['args'] = array();
 
 				foreach ( $callback['args'] as $key => $opts ) {
-					$arg_data             = array_intersect_key( $opts, $allowed_schema_keywords );
+					$arg_data             = array_intersect_key( (array) $opts, $allowed_schema_keywords );
 					$arg_data['required'] = ! empty( $opts['required'] );
 
 					$endpoint_data['args'][ $key ] = $arg_data;

This patch solves the issue for me.

#4 @slaFFik
3 years ago

@desrosj

Yes, and with the help of the developers of one of the active plugins on my site, we identified the plugin that is causing that issue. It will be patched, but I think that WordPress itself should also make sure that it's working with the array in the array_intersect_key() function.

Last edited 3 years ago by slaFFik (previous) (diff)

#5 @desrosj
3 years ago

  • Keywords needs-testing added

@apermo thanks for those details! I am also able to reproduce with the redux-framework plugin and looking into the issue some more.

@slaFFik I don't disagree, but it's important to understand what the patterns are causing the issue and confirm that type casting will not cause unintended consequences or backwards compatibility concerns before deciding on the correct change.

Are you able to share which plugin is causing the issue for you? If it's a custom plugin not available on the wordpress.org/plugin directory, dropping the offending code would be very helpful.

#6 follow-up: @slaFFik
3 years ago

@desrosj

The plugin is not hosted on wp.org.
Here is the code of that plugin that registers the endpoints:

public function register_api_endpoints() {

    register_rest_route( 'example/v1', '/some-content/woof/(?P<type>[a-zA-Z0-9-]+)', array(
        'methods'             => 'GET',
        'callback'            => array( $this, 'get_gutenberg_themes' ),
        'permission_callback' => function () {
            return current_user_can( 'edit_posts' );
        },
        'args'                => array(
            'type',
        ),
    ) );

    register_rest_route( 'example/v1', '/terms/(?P<slug>[a-zA-Z0-9-_]+)', array(
        'methods'             => 'GET',
        'callback'            => array( $this, 'get_taxonomy_terms' ),
        'permission_callback' => function () {
            return current_user_can( 'edit_posts' );
        },
        'args'                => array(
            'slug',
        ),
    ) );

    register_rest_route( 'example/v1', '/taxonomy/(?P<slug>[a-zA-Z0-9-_]+)', array(
        'methods'             => 'GET',
        'callback'            => array( $this, 'get_taxonomy' ),
        'permission_callback' => function () {
            return current_user_can( 'edit_posts' );
        },
        'args'                => array(
            'slug',
        ),
    ) );
}

It looks like it should be 'args' => [ 'type' => [] ] instead of 'args' => [ 'type' ] (same for slug).

Last edited 3 years ago by slaFFik (previous) (diff)

#7 in reply to: ↑ 6 @desrosj
3 years ago

  • Keywords needs-patch php8 added; needs-testing removed
  • Milestone changed from Awaiting Review to 5.6.1

It looks like it should be 'args' => [ 'type' => [] ] instead of 'args' => [ 'type' ] (same for slug).

Correct!

One additional note here, this pattern will cause a fatal error in PHP 8 due to strict typing now being applied to all internal PHP functionality.

Looks like this is a result of the efforts on #51020 (see the diff). Non-arrays were never truly supported, but the changes in #51020 seem to have surfaced warnings when being defined incorrectly.

I've been working through this with @TimothyBlynJacobs a bit in Slack.

The args is used primarily for validation, but with those strings set, it wouldn't be able to validate parameters. The get_data_for_route outputs the index. So the endpoint would "work" but wouldn't be doing validation properly and wouldn't be outputting correct schemas, but the route should actually be able to process the request and the response.

The type casting suggested above does not appear to be the correct solution here. While it would silence the warning, it would not force the correct behavior. A _doing_it_wrong() to notify developers of incorrect usage paired with skipping the item in the array when not the correct type seems like the safest way forward.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 years ago

This ticket was mentioned in Slack in #forums by timothybjacobs. View the logs.


3 years ago

#10 @AndrewNZ
3 years ago

I have just created a new ticket, #52008 when I perhaps should have made a comment on this ticket.

This ticket was mentioned in PR #802 on WordPress/wordpress-develop by aristath.


3 years ago
#11

  • Keywords has-patch added; needs-patch removed

#12 @aristath
3 years ago

  • Keywords needs-patch added; has-patch removed

I added a simple patch in https://github.com/WordPress/wordpress-develop/pull/802
The patch checks if the 1st argument is a string, and if it is then converts it to an array prior to array_intersect_key().

#13 @poena
3 years ago

I did a basic test of the patch by:
Installing the Redux plugin and confirming that the PHP warning was reproduced.
Applying the patch and confirming that the new doing it wrong message is displayed.
(PHP version: 7.4.11)

Last edited 3 years ago by poena (previous) (diff)

#14 @desrosj
3 years ago

#52008 was marked as a duplicate.

TimothyBJacobs commented on PR #802:


3 years ago
#15

@aristath thanks for the patch! I think I would prefer if we moved this to register_rest_route like we do for our other _doing_it_wrong notices. It should hopefully make it more clear about where the error is coming from and we can make sure it is only printed once per route. Something like

if ( count( array_filter( $args, 'is_array' ) ) !== count( $args ) {

}

#16 follow-up: @dovyp
3 years ago

The Redux issue has been resolved in version 4.1.24. A huge help to @TimothyBlynJacobs for his guidance. :)

Last edited 3 years ago by dovyp (previous) (diff)

aristath commented on PR #802:


3 years ago
#17

Thank you @TimothyBJacobs,
I moved the _doing_it_wrong call to register_rest_route. I did however leave the extra check in get_data_for_route as that will act as a safeguard in case a REST route is registered via rest_get_server()->register_route or other methods and not using the register_rest_route function.

#18 in reply to: ↑ 16 @apermo
3 years ago

Replying to dovyp:

The Redux issue has been resolved in version 4.1.24. A huge help to @TimothyBlynJacobs for his guidance. :)

Can confirm, it works, just updated my system. Thanks for your effort :)

#19 @hellofromTonya
3 years ago

  • Keywords has-patch added; reporter-feedback needs-patch removed

TimothyBJacobs commented on PR #802:


3 years ago
#20

Thanks for updating this @aristath! We should be checking $arg_group['args'] not $args though. Let's also make sure to include the namespace and route in the doing it wrong message so it will be cleared to the developers exactly which route registration is failing.

We can also make the check in get_data_for_route use is_array as well since any non-array value will cause PHP warnings, not just strings.

Lastly, would you be interested in writing unit tests for this? You can see \Tests_REST_API::test_register_route_with_missing_permission_callback_top_level_route for some examples of how to test register_rest_route with an invalid definition.

TimothyBJacobs commented on PR #802:


3 years ago
#21

Hey @aristath,

Just checking in about unit tests.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 years ago

#23 @whyisjake
3 years ago

As we prep for the 5.6.1 release, we are going to punt this to the 5.6.2 milestone as we still need some work and tests.

#24 @audrasjb
3 years ago

  • Milestone changed from 5.6.1 to 5.6.2

Given 5.6.1, let's move this ticket to 5.6.2 to give it some more time.

#25 @desrosj
3 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from 5.6.2 to Future Release

5.6.2 RC is going to be packaged in a few hours. It seems that the PR has an open request for some unit tests, so I'm going to punt to Future Release.

aristath commented on PR #802:


3 years ago
#26

@TimothyBJacobs I tried but I'm afraid I don't understand how to write unit tests for that.
If you want I can just close this PR so you can create another one with tests included.

#27 follow-up: @Hinjiriyo
3 years ago

  • Severity changed from normal to critical

The fatal error still appears in WP 5.8. When I try to call a post edit page, PHP throws in debug mode the following lines:

Fatal error: Uncaught TypeError: array_intersect_key(): Argument #1 ($array) must be of type array, string given in /wp-includes/rest-api/class-wp-rest-server.php:1436 
Stack trace: 
#0 /wp-includes/rest-api/class-wp-rest-server.php(1436): array_intersect_key('footer', Array) 
#1 /wp-includes/rest-api/class-wp-rest-server.php(1358): WP_REST_Server->get_data_for_route('/gridd/v1/parti...', Array, 'view') 
#2 /wp-includes/rest-api/class-wp-rest-server.php(1224): WP_REST_Server->get_data_for_routes(Array, 'view') 
#3 /wp-includes/rest-api/class-wp-rest-server.php(1140): WP_REST_Server->get_index(Object(WP_REST_Request)) 
#4 /wp-includes/rest-api/class-wp-rest-server.php(987): WP_REST_Server->respond_to_request(Object(WP_REST_Request), '/', Array, NULL) 
#5 /wp-includes/rest-api.php(495): WP_REST_Server->dispatch(Object(WP_REST_Request)) 
#6 /wp-includes/rest-api.php(2832): rest_do_request(Object(WP_REST_Request)) 
#7 [internal function]: rest_preload_api_request(Array, '/') 
#8 /wp-includes/block-editor.php(444): array_reduce(Array, 'rest_preload_ap...', Array) 
#9 /wp-admin/edit-form-blocks.php(68): block_editor_rest_api_preload(Array, Object(WP_Block_Editor_Context)) 
#10 /wp-admin/post.php(187): require('/www/htdocs/...') 
#11 {main} thrown in /wp-includes/rest-api/class-wp-rest-server.php on line 1436

The error appears even no plugin is active. I didn't check whether the current theme ("Gridd") is the source of the error.

In the file /wp-includes/rest-api/class-wp-rest-server.php in line 1436 the variable $opts sometimes contains a string. Since it is used in array_intersect_key() which expects an array, an error is raised.

With a little debugging $opts yields on the current installation in the loop sth. like:

$opts: 'footer'
$opts: 'sidebar_1'
$opts: 'sidebar_2'
$opts: 'sidebar_3'

I inserted a quick workaround before line 1436 to avoid the error:

if ( ! is_array( $opts ) ) {
        continue;
}

Now editing a post and page is possible.

That is just a quick approach without any tests for side effects. And it will be gone after an WP upgrade. So I'm warning about using that fix. I'm glad if there would be a better solution.

Last edited 3 years ago by Hinjiriyo (previous) (diff)

#28 @jrf
3 years ago

  • Severity changed from critical to normal

#29 in reply to: ↑ 27 @johnmark8080
3 years ago

Thanks. Just noticed this error this week when editing posts, couldn't tie it back to any plugins. Applied the quick, temporary fix from @Hinjiriyo. No errors now (but what was trying and failing to the rest_api from the edit page?). It was more of a nuisance for me as it was possible to edit posts before and after applying the fix. I just didn't want users to be distressed by the error message. Hoping for a permanent fix soon.

#30 @nhadsall
23 months ago

I'd love to see some movement on this, since we have a client impacted by it as well. I'm unable to add the unit tests. Can someone else help with that?

#31 @SergeyBiryukov
23 months ago

  • Milestone changed from Future Release to 6.1

@nateallen
21 months ago

Unit test confirming fix for bug

nate-allen commented on PR #802:


21 months ago
#32

I added a unit test based on the code from this PR .

#33 @nateallen
21 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

TimothyBJacobs commented on PR #802:


21 months ago
#34

Thanks @nate-allen! Could you create a new PR with @aristath's changes plus your unit test?

This ticket was mentioned in PR #2970 on WordPress/wordpress-develop by nate-allen.


21 months ago
#35

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

Adds unit test for #802

This ticket was mentioned in Slack in #core by chaion07. View the logs.


19 months ago

This ticket was mentioned in Slack in #core by chaion07. View the logs.


19 months ago

#38 @chaion07
19 months ago

@slaFFik thank you for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we feel the need for a review from a Component Maintainer or Committer on the patch as well as the unit test. Thanks!

Props to @robinwpdeveloper

#39 @davidbaumwald
18 months ago

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

In 54339:

REST API: Ensure args is an array of arrays in register_rest_route().

When calling register_rest_route(), the args parameter for a route should be an array of arrays. However, some plugins/themes have passed an array of strings or key-value pairs which produces a PHP warning when array_intersect_key is used to filter the array keys based on an allowed list of schema keywords.

This change adds a check of the args parameter to ensure it's an array of arrays, presenting a _doing_it_wrong if any element of args is not an array and restructuring to an array of arrays. This change also adds a unit test for the incorrect usage described above, expecting that a _doing_it_wrong is produced.

Props slaFFik, desrosj, apermo, AndrewNZ, aristath, poena, dovyp, timothyblynjacobs, Hinjiriyo, johnmark8080, nateallen.
Fixes #51986.

dream-encode commented on PR #2970:


18 months ago
#41

Thanks for the PR! This was merged into core in https://core.trac.wordpress.org/changeset/54339. 🎉

#42 @TobiasBg
18 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
if ( count( array_filter( $arg_group['args'], 'is_array' ) ) !== count( $arg_group['args'] ) ) {

as a custom implementation of an any() function is not very readable and not that nice performance-wise (array_filter() will always loop the full array, even though we are only interested in finding one (the first) non-array).

How about a simple foreach that is left early once a match is found?:

foreach( $arg_group['args'] as $arg ) {
        if ( ! is_array( $arg ) ) {
                _doing_it_wrong(
                        __FUNCTION__,
                        sprintf(
                                /* translators: %s: The REST API route being registered. */
                                __( 'REST API $args should be an array of arrays. Non-array value detected for %s.' ),
                                '<code>' . $clean_namespace . '/' . trim( $route, '/' ) . '</code>'
                        ),
                        '6.1.0'
                );
                break; // Leave the foreach loop once one non-array argument was found.
        }
}

instead?

#43 @SergeyBiryukov
18 months ago

In 54346:

I18N: Move code out of a translatable string in register_rest_route().

To simplify the string and exclude any parts that don't require translation, $args can be moved out of the string and added as a placeholder.

Follow-up to [54339].

See #51986.

#44 @TobiasBg
18 months ago

Updated suggestion after the change in the translatable string in [54346]:

<?php
foreach( $arg_group['args'] as $arg ) {
        if ( ! is_array( $arg ) ) {
                _doing_it_wrong(
                        __FUNCTION__,
                        sprintf(
                                /* translators: 1: $args, 2: The REST API route being registered. */
                                __( 'REST API %1$s should be an array of arrays. Non-array value detected for %2$s.' ),
                                '<code>$args</code>',
                                '<code>' . $clean_namespace . '/' . trim( $route, '/' ) . '</code>'
                        ),
                        '6.1.0'
                );
                break; // Leave the foreach loop once one non-array argument was found.
        }
}

This ticket was mentioned in Slack in #core by desrosj. View the logs.


18 months ago

#46 @desrosj
18 months ago

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

@TobiasBg would you mind opening a new ticket for this improvement? This was discussed today in a final bug scrub before RC1 and though it would be nice, it was deemed not a blocker for 6.1.

#47 @TobiasBg
18 months ago

Sure! I opened #56804 as a follow-up ticket.

Note: See TracTickets for help on using tickets.