WordPress.org

Make WordPress Core

Opened 5 months ago

Closed 4 weeks ago

Last modified 37 hours ago

#51056 closed defect (bug) (fixed)

Fetch_feed parsing of permalinks triggers simplepie preg_match warnings

Reported by: litemotiv Owned by: iandunn
Milestone: 5.6.1 Priority: high
Severity: critical Version: 5.5
Component: External Libraries Keywords: php8 has-patch has-unit-tests
Focuses: rest-api Cc:

Description

Tested with 2 sites running within the same 5.5 multisite network:

$feed = fetch_feed(<other WP site RSS URL>);
$items = $feed->get_items(0,2);

foreach ($items as $item)
    echo $item->get_permalink();

PHP warnings:

 *6667000 FastCGI sent in stderr: "PHP message: PHP Warning:  preg_match() expects parameter 2 to be string, array given in /media/www/multisite/wp-includes/class-simplepie.php on line 2620
PHP message: PHP Warning:  preg_match() expects parameter 2 to be string, array given in /media/www/multisite/wp-includes/class-simplepie.php on line 2620
PHP message: PHP Warning:  preg_match() expects parameter 2 to be string, array given in /media/www/multisite/wp-includes/class-simplepie.php on line 2620
PHP message: PHP Warning:  preg_match() expects parameter 2 to be string, array given in /media/www/multisite/wp-includes/class-simplepie.php on line 2620
[ ... ]

The permalinks are parsed okay and are returned.

Attachments (1)

51056.diff (5.4 KB) - added by david.binda 4 months ago.

Download all attachments as: .zip

Change History (21)

This ticket was mentioned in PR #496 on WordPress/wordpress-develop by inc2734.


5 months ago

  • Keywords has-patch added

Display warning when using category/tag archives feeds.
https://core.trac.wordpress.org/ticket/51056

#2 @inc2734
5 months ago

I've recreated it too. It happens when you use the feed on the archive page. I created a pull request.
https://github.com/WordPress/wordpress-develop/pull/496

#3 @david.binda
4 months ago

I've been able to reproduce the issue as well. As it was already mentioned, the issue is easily reproducible when attempting to parse a category feed on a WordPress install.

What happens is that WordPress sends 2 link HTTP headers in the response via rest_output_link_header function (one for the api.wp.org and one alternate for application/json leading to the rest api endpoint).

SimplePie used by the fetch_feed function, however, expects the multiple headers of the same name being joined together and separated by a comma ( a notation described by rfc8288 ). SimplePie itself handles this in https://github.com/simplepie/simplepie/blob/a72e1dfafe7870affdae3edf0d9a494e4fa31bc6/library/SimplePie/HTTP/Parser.php#L254,L263

However the SimplePie's SimplePie_File class is being overloaded by WP_SimplePie_File via SimplePie::set_file_class in https://core.trac.wordpress.org/browser/trunk/src/wp-includes/feed.php?rev=48435#L805 , which does not handle the headers in the same way (multiple headers of the same name are being stored in an array).

IMHO, it would be better to fix the issue in WP_SimplePie_File, not touching the wp-includes/class-simplepie.php file, which is a copy of an external project, which does not suffer from the issue.

Further, the SimplePie_File class makes sure that only the last content-type header is being used. While this works as expected in the current implementation of the WP_SimplePie_File class, I believe that it might be a good idea to cover that with a unit test.

@david.binda
4 months ago

#4 @david.binda
4 months ago

I forgot to add, that the warning is new, as it was introduced in terms of the SimplePie update in WP 5.5 ( r47733 )

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


6 weeks ago

#6 @iandunn
6 weeks ago

Related or possible duplicate: #51956

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


6 weeks ago

#8 @SergeyBiryukov
6 weeks ago

  • Milestone changed from Awaiting Review to 5.6.1

Moving to 5.6.1 along with #51956.

Per comment:4, the warning is the result of updating SimplePie to version 1.5.5 for WordPress 5.5 in [47733] / #36669.

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


5 weeks ago

#10 @iandunn
5 weeks ago

#51956 was marked as a duplicate.

#11 @iandunn
5 weeks ago

  • Focuses rest-api added
  • Keywords php8 added
  • Owner set to iandunn
  • Priority changed from normal to high
  • Severity changed from normal to critical
  • Status changed from new to assigned

Bringing over keywords/etc from #51956.

Additional props from that ticket: NicolasKulka, hellofromTonya, mbabker, skithund , SergeyBiryukov, desrosj, timothyblynjacobs

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


4 weeks ago

  • Keywords has-unit-tests added

SimplePie combines multiple values for a HTTP header into a comma-separated string, but WP_SimplePie_File was leaving them as an array. That leads to a fatal error in PHP 8.

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

#13 @iandunn
4 weeks ago

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

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.

#15 follow-up: @iandunn
4 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to 5.6.1.

@desrosj, @SergeyBiryukov, is one of you able to look at that?

ticket:51956#comment:17 has an mu-plugin that helps w/ manual testing.

#16 in reply to: ↑ 15 @SergeyBiryukov
4 weeks ago

Replying to iandunn:

is one of you able to look at that?

Thanks for getting to the bottom of this!

One note I have is that implode() should be used instead of join(), see [49193] / #50767.

Otherwise, [49803] looks good to backport.

#17 @iandunn
4 weeks ago

Ah, thanks for catching that!

I'll fix it, and then go ahead and backport since you've already reviewed.

#18 @iandunn
4 weeks ago

In 49805:

Feed: Replace join() with implode() for safety.

Canonical functions should be used instead of aliases, because aliases can be deprecated or removed withough much notice. See r49193.

Props SergeyBiryukov.
See #51056.

#19 @iandunn
4 weeks ago

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

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.

Note: See TracTickets for help on using tickets.