Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#51956 closed defect (bug) (duplicate)

Fatal error with PHP8 in header link RSS feed on dashboard widget

Reported by: nicolaskulka's profile NicolasKulka Owned by:
Milestone: Priority: high
Severity: critical Version:
Component: Widgets Keywords: php8 has-patch
Focuses: rest-api Cc:

Description

I just found an issue with a native WordPress RSS feed on a category.

It returns 2 different links via the rest_output_link_wp_head function in the wp-includes/rest-api.php file.

Example : https://www.wpserveur.net/securite-wps/feed/
Link: <https://www.wpserveur.net/wp-json/>; rel="https://api.w.org/"
Link: <https://www.wpserveur.net/wp-json/wp/v2/categories/45>; rel="alternate";

I created a widget on the dashboard via this rss feed with the wp_dashboard_primary_output function.

With PHP 7.4 => Warning: preg_match() expects parameter 2 to be string, array given in /wp-includes/class-simplepie.php on line 2620

With PHP 8.0 => Fatal error: Uncaught TypeError: preg_match(): Argument #2 ($subject) must be of type string, array given in /wp-includes/class-simplepie.php:2620

Change History (27)

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


3 years ago

#2 @NicolasKulka
3 years ago

Same problem here => #51056

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

#3 @iandunn
3 years ago

  • Keywords php8 added

Hi, thanks for reporting that.

It sounds like you encountered this error with a plugin, is that right? Can you share the full code? If you're seeing a stack trace, please share that too. I'm not sure how to reproduce it yet.

#4 @NicolasKulka
3 years ago

To reproduce the problem :

add_action( 'wp_dashboard_setup', 'add_dashboard_widget_wpsecu' );
	function add_dashboard_widget_wpsecu() {
		// Add dashboard Widget
		wp_add_dashboard_widget( 'dashboard_widget_wpsecu', __( 'Sécurité de WordPress' ), 'dashboard_widget_function_secu' );

		// Sort
		global $wp_meta_boxes;
		$normal_dashboard               = $wp_meta_boxes['dashboard']['normal']['core'];
		$dashboard_widget_wpsecu_backup = array( 'dashboard_widget_wpsecu' => $normal_dashboard['dashboard_widget_wpsecu'] );
		unset( $normal_dashboard['dashboard_widget_wpsecu'] );
		$sorted_dashboard                             = array_merge( $dashboard_widget_wpsecu_backup, $normal_dashboard );
		$wp_meta_boxes['dashboard']['normal']['core'] = $sorted_dashboard;
	}

	function dashboard_widget_function_secu() {
		$feeds = array(
			'news' => array(
				'link'         => 'https://www.wpserveur.net/securite-wps/',
				'url'          => 'https://www.wpserveur.net/securite-wps/feed/',
				'title'        => 'Securite WordPress',
				'items'        => 3,
				'show_summary' => 1,
				'show_author'  => 0,
				'show_date'    => 1,
			)
		);
		wp_dashboard_primary_output( 'dashboard_widget_wpsecu', $feeds );
	}
Last edited 3 years ago by NicolasKulka (previous) (diff)

#5 @afragen
3 years ago

Please confirm which version of WordPress is being used. I thought this or a similar error was fixed in RC2 or thereabouts.

#6 @NicolasKulka
3 years ago

I am using the latest WP 5.5 version with the PHP7.4 version for the warning.

For the fatal error, I am using the latest RC of version WP5.6 with version PHP8.0

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


3 years ago

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


3 years ago

#9 @mbabker
3 years ago

Seems like an unreported bug in the upstream SimplePie package, see https://github.com/simplepie/simplepie/blob/1c68e14ca3ac84346b6e6fe3c5eedf725d0f92c6/library/SimplePie.php#L2583-L2585 for the same code in the upstream package.

#10 @iandunn
3 years ago

In ticket:51056#comment:3 @david.binda suggested that we should fix this internally, because SimplePie wasn't affected. The patch there doesn't seem to fix this problem, though. I'm not sure yet if this is a duplicate, or just related.

This ticket was mentioned in PR #795 on WordPress/wordpress-develop by iandunn.


3 years ago
#11

  • Keywords has-patch added; needs-patch removed

This is still a WIP, but this demonstrates one possible fix.

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

TimothyBJacobs commented on PR #795:


3 years ago
#13

🤦 I misread your comment as that patch did work, never mind!

iandunn commented on PR #795:


3 years ago
#14

No worries :)

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


3 years ago

#16 @iandunn
3 years ago

  • Component changed from General to Widgets
  • Focuses rest-api added
  • Milestone changed from Awaiting Review to 5.6.1
  • Priority changed from normal to high
  • Severity changed from normal to critical
  • Status changed from new to assigned

Assigning to 5.6.1, since there isn't enough time for fit this in for 5.6, and the full impact/solution isn't clear yet.

xref https://wordpress.slack.com/archives/C02RQBWTW/p1607372723178400
xref https://make.wordpress.org/core/2020/11/23/wordpress-and-php-8-0/

#17 @iandunn
3 years ago

This may be a duplicate after all.

I've always had lots of problems with the #43055 build tool, and got frustrated enough yesterday that I tried out the #44492 method again. Now that I'm running directly from src/, 51056.diff does fix this for me.

@hellofromtonya, @SergeyBiryukov, I'm wondering if that could have been part of the reason the patch didn't work for you either? See #51960 for some details.

Another reason might be because the transients were cached?

Here's the mu-plugins I'm using to make these easier to test. They effectively prevent the feeds from being cached, so you don't have to manually delete the transients. (You'll still have to delete any existing ones before running this, though).

51056.php:

<?php

// don't cache results, so can easily test changes
add_filter( 'wp_feed_cache_transient_lifetime', function() {
        return 1;
} );

add_action( 'template_redirect', function() {
        // it only happens on some feeds, isolate which ones and figure out why
        $urls = array(
                // these are fine
                'https://wordpress.org/news/category/releases/feed/',
                'https://iandunn.name/tag/z/feed/',
                'https://wptavern.com/2020/12/feed/',
                'https://heropress.com/feed/',

                // these cause errors, b/c they have multiple 'Link' headers
                'https://www.wpserveur.net/securite-wps/feed/',
        );

        foreach ( $urls as $url ) {
                $feed = fetch_feed( $url );

                $items = $feed->get_items( 0, 2 );

                foreach ( $items as $item ) {
                        echo $item->get_permalink() . "\n";
                }
        }

        wp_die(__FILE__);
} );

51956.php:

<?php

// don't cache results, so can easily test changes
add_filter( 'wp_feed_cache_transient_lifetime', function() {
        return 1;
} );

add_action( 'wp_dashboard_setup', 'add_dashboard_widget_wpsecu' );

function add_dashboard_widget_wpsecu() {
        wp_add_dashboard_widget( 'dashboard_widget_wpsecu', __( 'Sécurité de WordPress' ), 'dashboard_widget_function_secu' );
}

function dashboard_widget_function_secu() {
        $feeds = array(
                'news'   => array(
                        // should get fatal w/ these b/c category
                        'link'         => 'https://www.wpserveur.net/securite-wps/',
                        'url'          => 'https://www.wpserveur.net/securite-wps/feed/',

                        // shouldn't get fatal w/ these b/c not category
//                      'link'         => 'https://www.wpserveur.net/',
//                      'url'          => 'https://www.wpserveur.net/feed/',

                        'title'        => apply_filters( 'dashboard_primary_title', __( 'WordPress Blog' ) ),
                        'items'        => 2,
                        'show_summary' => 0,
                        'show_author'  => 0,
                        'show_date'    => 0,
                ),
        );

        wp_dashboard_primary_output( 'dashboard_widget_wpsecu', $feeds );
}

#18 @NicolasKulka
3 years ago

ye, function 'rest_output_link_wp_head' return two links

/**
 * Outputs the REST API link tag into page header.
 *
 * @since 4.4.0
 *
 * @see get_rest_url()
 */
function rest_output_link_wp_head() {
	$api_root = get_rest_url();

	if ( empty( $api_root ) ) {
		return;
	}

	printf( '<link rel="https://api.w.org/" href="%s" />', esc_url( $api_root ) );

	$resource = rest_get_queried_resource_route();

	if ( $resource ) {
		printf( '<link rel="alternate" type="application/json" href="%s" />', esc_url( rest_url( $resource ) ) );
	}
}

This function 'rest_get_queried_resource_route' add a second link, add WP 5.5

/**
 * Gets the REST route for the currently queried object.
 *
 * @since 5.5.0
 *
 * @return string The REST route of the resource, or an empty string if no resource identified.
 */
function rest_get_queried_resource_route() {
	if ( is_singular() ) {
		$route = rest_get_route_for_post( get_queried_object() );
	} elseif ( is_category() || is_tag() || is_tax() ) {
		$route = rest_get_route_for_term( get_queried_object() );
	} elseif ( is_author() ) {
		$route = '/wp/v2/users/' . get_queried_object_id();
	} else {
		$route = '';
	}

	/**
	 * Filters the REST route for the currently queried object.
	 *
	 * @since 5.5.0
	 *
	 * @param string $link The route with a leading slash, or an empty string.
	 */
	return apply_filters( 'rest_queried_resource_route', $route );
}
Last edited 3 years ago by NicolasKulka (previous) (diff)

#19 @iandunn
3 years ago

Assuming this is a duplicate, it seems like 51056.diff is a better solution that PR 795.

It seems valid for SimplePie's get_links() to expect a string, since it's parser combines multiple headers into a comma-separated string. That doesn't happen in Core b/c we override the File handler, though.

https://core.trac.wordpress.org/ticket/51056#comment:3 has a more detailed explanation. It just took the build tool realization and a 6th reading for it to all fit together :)

#20 @hellofromTonya
3 years ago

Using the Dashboard (using the custom widget by the reporter) and front-end with the mu-plugin code Ian provided:

  • before: fatal error in the Dashboard and front-end as well as in debug.log
  • after 51056.diff: no fatal error and works as expected
  • after PR 795: no fatal error and works as expected

Both the PR in this ticket and the patch in #51056 resolve the warning (< PHP8) and fatal error (PHP 8).

I agree with @iandunn that this ticket is a duplicate of #51056, whereas 50156 reports the PHP warning for < PHP 8 and this ticket reports the fatal error for PHP 8.

Which solution is better?

IMO 50156.diff is a better solution.

Why?

The patch directly handles the conversion to the expected comma-separated string at the point where the array or Requests_Utility_CaseInsensitiveDictionary (dictionary) is returned from rest_output_link_header().

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

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


3 years ago

#22 @iandunn
3 years ago

  • Milestone 5.6.1 deleted
  • Resolution set to duplicate
  • Status changed from assigned to closed

Duplicate of #51056.

#23 @iandunn
3 years ago

In 49803:

Feed: Merge multiple header values to avoid fatal error.

When SimplePie parses HTTP headers, it combines multiple values for the same header into a comma-separated string. WP_SimplePie_File overrides the parsing, but was leaving them as an array instead.

That lead to a fatal error in PHP 8, because other parts of the codebase ended up passing an array to a function that expected a string.

Props david.binda, litemotiv, inc2734, NicolasKulka, hellofromTonya, mbabker, skithund, SergeyBiryukov, desrosj, timothyblynjacobs.
Fixes #51056. See #51956.

#24 @iandunn
3 years ago

In 49806:

Feed: Merge multiple header values to avoid fatal error.

When SimplePie parses HTTP headers, it combines multiple values for the same header into a comma-separated string. WP_SimplePie_File overrides the parsing, but was leaving them as an array instead.

That lead to a fatal error in PHP 8, because other parts of the codebase ended up passing an array to a function that expected a string.

Props david.binda, litemotiv, inc2734, NicolasKulka, hellofromTonya, mbabker, skithund, SergeyBiryukov, desrosj, timothyblynjacobs.
Reviewed by SergeyBiryukov, iandunn.
Merges [49803] and [49805] to the 5.6 branch.
Fixes #51056. See #51956.

#25 @Znuff
3 years ago

49806 doesn't seem to fix the issue under PHP 8.0(.1).

I've applied the diff, I even replaced the full file (just in case I screwed up with the diff), but the issue is still present.

This is also a warning in PHP 7.4 (not a fatal error).

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

#26 @iandunn
3 years ago

Hi @Znuff, what version of WP did you apply the diff to?

Can you reproduce it on a clean install? If so, can you open a new ticket with the reproduction steps, copy/paste the exact error output, etc? Include a link to this ticket as background info.

Note: See TracTickets for help on using tickets.