Make WordPress Core

Opened 5 months ago

Closed 4 months ago

#59841 closed defect (bug) (invalid)

Post 6.4 Upgrade Fatal:

Reported by: rebasaurus's profile rebasaurus Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: REST API Keywords:
Focuses: Cc:

Description

After upgrading to 6.4, this fatal occurs:

PHP message: Fatal error: Uncaught TypeError: Cannot access offset of type string on string in /var/www/wp-includes/rest-api.php:3082
Stack trace:
#0 /var/www/wp-includes/rest-api.php(3104): rest_default_additional_properties_to_false('string')
#1 /var/www/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php(484): rest_default_additional_properties_to_false(Array)
#2 /var/www/wp-includes/rest-api/fields/class-wp-rest-meta-fields.php(511): WP_REST_Meta_Fields->get_registered_fields()
#3 /var/www/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php(768): WP_REST_Meta_Fields->get_field_schema()
#4 /var/www/wp-includes/rest-api/endpoints/class-wp-rest-controller.php(388): WP_REST_Revisions_Controller->get_item_schema()
#5 /var/www/wp-includes/rest-api/endpoints/class-wp-rest-controller.php(343): WP_REST_Controller->get_context_param()
#6 /var/www/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php(783): WP_REST_Controller->get_collection_params()
#7 /var/www/wp-includes/rest-api/endpoints/class-wp-rest-revisions-controller.php(97): WP_REST_Revisions_Controller->get_collection_params()
#8 /var/www/wp-includes/rest-api.php(250): WP_REST_Revisions_Controller->register_routes()
#9 /var/www/wp-includes/class-wp-hook.php(324): create_initial_rest_routes(Object(WP_REST_Server))
#10 /var/www/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array)
#11 /var/www/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
#12 /var/www/wp-includes/rest-api.php(593): do_action('rest_api_init', Object(WP_REST_Server))
#13 /var/www/wp-includes/rest-api.php(417): rest_get_server()
#14 /var/www/wp-includes/class-wp-hook.php(324): rest_api_loaded(Object(WP))
#15 /var/www/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array)
#16 /var/www/wp-includes/plugin.php(565): WP_Hook->do_action(Array)
#17 /var/www/wp-includes/class-wp.php(418): do_action_ref_array('parse_request', Array)
#18 /var/www/wp-includes/class-wp.php(813): WP->parse_request('')
#19 /var/www/wp-includes/functions.php(1336): WP->main('')
#20 /var/www/wp-blog-header.php(16): wp()
#21 /var/www/index.php(17): require('/var/www/wp-blo...')
#22 {main}
  thrown in /var/www/wp-includes/rest-api.php on line 3082

I suspect introduced in https://core.trac.wordpress.org/changeset/56819. When $schema is of the below format and is passed into https://github.com/WordPress/WordPress/blob/436d55fa51a2f46ad6ac7e16fcb4e83618afb030/wp-includes/rest-api.php#L3081:

array (
  'type' => 'array',
  'items' => array (
    'type' => 'string',
  ),
)

It recursively calls itself again at https://github.com/WordPress/WordPress/blob/436d55fa51a2f46ad6ac7e16fcb4e83618afb030/wp-includes/rest-api.php#L3104 with the 'items' array and throws the fatal. An example post type is attachment where I'm seeing it occur on.

Change History (12)

#1 @rajinsharwar
5 months ago

Hi @rebasaurus, thanks for reporting the issue. Could you please share replication steps on how to reproduce this Fatal error?

#2 @rebasaurus
5 months ago

Sure, you can simply shell in and do:

rest_default_additional_properties_to_false( [ 'type' => 'array', 'description' => 'array of strings for screenshot URLs', 'default' => [], 'items' => 'string' ] );

To be clear this happens on WP 6.3, when you directly call the function, but I noticed that class-wp-rest-template-revisions-controller.php was newly added on 6.4. In that class, get_registered_fields() is called now, which calls rest_default_additional_properties_to_false(). I wasn't sure if ya'll wanted to do any additional handling for unexpected formatted schemas?

		register_post_meta(
			'example_post_type',
			'example_meta_key',
			array(
				'type'              => 'array',
				'description'       => 'Array of strings',
				'sanitize_callback' => function ( $urls ) {
					return array_map( fn( $url ) => sanitize_text_field( $url ), $urls );
				},
				'show_in_rest'      => array(
					'schema' => array(
						'type'  => 'array',
						'items' => 'string',
					),
				),
			)
		);
Version 7, edited 5 months ago by rebasaurus (previous) (next) (diff)

#3 @rebasaurus
5 months ago

I copied and pasted the wrong example in the original description :facepalm:. I meant for the below $schema to be where it triggers the fatal:

array (size=4)
  'type' => string 'array' (length=5)
  'description' => string 'Array of strings' (length=40)
  'default' => 
    array (size=0)
      empty
  'items' => string 'string' (length=6)

When rest_default_additional_properties_to_false() gets recursively called, it assumes $schema['items'] is an array, but in this case, it's a string.

Last edited 5 months ago by rebasaurus (previous) (diff)

#4 @hellofromTonya
5 months ago

  • Milestone changed from Awaiting Review to 6.4.1

Hello @rebasaurus,

Welcome back to WordPress Core's Trac :) Thank you for reporting this issue.

Moving it into 6.4.1, dropping it into the Make/Core #core slack channel for more immediate awareness, and pinging some of the REST API maintainers @TimothyBlynJacobs @kadamwhite @spacedmonkey.

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


5 months ago

#6 @hellofromTonya
5 months ago

Summary of discussion in Make/Core slack:

@TimothyBlynJacobs noted:

This looks like user error to me.

'schema' => array(
	'type'  => 'array',
	'items' => 'string',
),

That is an invalid JSON schema.

Restoring this back to Awaiting Review.

Timothy also noted:

We could probably consider hardening those functions to behave better with invalid schemas along the lines of some of the other PHP 8 related changes we are making.

#7 @hellofromTonya
5 months ago

  • Milestone changed from 6.4.1 to Awaiting Review

#8 @jorbin
5 months ago

  • Milestone changed from Awaiting Review to 6.4.2

Moving to 6.4.2 so we can consider follow up but based on the comment above, it's not urgent.

#9 @jorbin
5 months ago

  • Keywords reporter-feedback added

Adding reporter feedback since we are still waiting, but barring any I think this should be closed as invalid. I think the absence of widespread reports of this issue means the assumption that it is an implementation error is correct.

#10 @SergeyBiryukov
4 months ago

  • Milestone changed from 6.4.2 to 6.4.3

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


4 months ago

#12 @jorbin
4 months ago

  • Keywords reporter-feedback removed
  • Milestone 6.4.3 deleted
  • Resolution set to invalid
  • Status changed from new to closed

With no further information, I'm closing this as invalid. If there is new information, then this should be reconsidered but as of now, the assumption is that this is an implementation error rather than a core issue.

Note: See TracTickets for help on using tickets.