#58798 closed defect (bug) (fixed)
Fix possible PHP warning in /wp-includes/feed.php
Reported by: | zahardoc | Owned by: | johnjamesjacoby |
---|---|---|---|
Milestone: | 6.7 | Priority: | normal |
Severity: | normal | Version: | 6.2.2 |
Component: | Feeds | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
If some plugins or themes use "enclosure" meta field as a one-line string, it results in PHP warnings:
PHP Warning: Undefined array key 2 in /wp-includes/feed.php on line 484
PHP Warning: Undefined array key 1 in /wp-includes/feed.php on line 494
It happens because there is no check of string content there:
foreach ( (array) get_post_custom() as $key => $val ) { if ( 'enclosure' === $key && is_array( $val ) ) { foreach ( (array) $val as $enc ) { $enclosure = explode( "\n", $enc ); / Only get the first element, e.g. 'audio/mpeg' from 'audio/mpeg mpga mp2 mp3'. $t = preg_split( '/[ \t]/', trim( $enclosure[2] ) ); $type = $t[0]; /** * Filters the RSS enclosure HTML link tag for the current post. * * @since 2.2.0 * * @param string $html_link_tag The HTML link tag with a URI and other attributes. */ echo apply_filters( 'rss_enclosure', '<enclosure url="' . esc_url( trim( $enclosure[0] ) ) . '" length="' . absint( trim( $enclosure[1] ) ) . '" type="' . esc_attr( $type ) . '" />' . "\n" ); } } }
Proposal:
Add checks if the enclosure is an array, and each item can be exploded into exactly 3 items:
if ( 'enclosure' === $key && is_array( $val ) ) {}
And this:
$enclosure = explode( "\n", $enc ); if ( 3 !== count( $enclosure ) ) { continue; }
Attachments (1)
Change History (16)
This ticket was mentioned in Slack in #core by yui. View the logs.
12 months ago
#3
@
6 months ago
Any update on a fix for this issue? It's really cluttering up our debug logs and making it hard to see actual issues.
This ticket was mentioned in PR #6643 on WordPress/wordpress-develop by @narenin.
5 months ago
#4
- Keywords has-patch added; needs-patch removed
Trac ticket: https://core.trac.wordpress.org/ticket/58798
#5
follow-up:
↓ 8
@
2 months ago
- Milestone changed from Awaiting Review to 6.7
- Owner set to johnjamesjacoby
- Status changed from new to assigned
Hi @zahardoc, thanks for making this ticket here.
I've personally confirmed your findings, and would like to get this fixed in the next major release.
@nihar007, your patch appears to include some unintended changes, so my plan is to base an updated patch on the Pull Request from @narenin on GitHub (though you'll still both get props for contributing to the fix).
For added context, I was seeing this on a site that was running the Seriously Simple Podcasting plugin, because is uses the enclosure
post-meta key with only a single-line entry, rather than the multi-line format that WordPress is coded to expect (see: do_enclose()
).
Additionally, I think the same fix should be applied to both the rss_enclosure()
and atom_enclosure()
functions.
Lastly, these functions are ooooold, so I'll be resisting the urge to do any other improvements to the surrounding code, hugging closely to the suggested changes already here.
#6
@
5 weeks ago
@johnjamesjacoby Do you think you will have a chance to follow this up prior to the release candidate in a couple of weeks?
#8
in reply to:
↑ 5
@
4 weeks ago
Replying to johnjamesjacoby:
Hi @zahardoc, thanks for making this ticket here.
I've personally confirmed your findings, and would like to get this fixed in the next major release.
@nihar007, your patch appears to include some unintended changes, so my plan is to base an updated patch on the Pull Request from @narenin on GitHub (though you'll still both get props for contributing to the fix).
For added context, I was seeing this on a site that was running the Seriously Simple Podcasting plugin, because is uses the
enclosure
post-meta key with only a single-line entry, rather than the multi-line format that WordPress is coded to expect (see:do_enclose()
).
Additionally, I think the same fix should be applied to both the
rss_enclosure()
andatom_enclosure()
functions.
Lastly, these functions are ooooold, so I'll be resisting the urge to do any other improvements to the surrounding code, hugging closely to the suggested changes already here.
Hi @johnjamesjacoby
I found these other functions that this fix could be applied:
- wp_xmlrpc_server::_prepare_post
- wp_xmlrpc_server::mw_getPost
Let me know if you need some help. I think I can write some unit tests for these functions.
This ticket was mentioned in PR #7557 on WordPress/wordpress-develop by @Rahmohn.
4 weeks ago
#9
- Keywords has-unit-tests added
This PR checks the size of enclosure
meta field in rss_enclosure()
function and add some tests to this function.
Trac ticket: https://core.trac.wordpress.org/ticket/58798
This ticket was mentioned in PR #7557 on WordPress/wordpress-develop by @Rahmohn.
4 weeks ago
#10
This PR checks the size of enclosure
meta field in rss_enclosure()
function and add some tests to this function.
Trac ticket: https://core.trac.wordpress.org/ticket/58798
#11
@
4 weeks ago
I've created a PR to fix this possible PHP warning. Also, I created some tests for the function rss_enclosure()
.
I noticed that the function atom_enclosure()
doesn't need to be applied the same fix because this function sets default values when the enclosure
meta field is not stored in a multiline string i.e. this function checks if the keys exist before accessing them:
<?php // Parse URL. if ( isset( $enclosure[0] ) && is_string( $enclosure[0] ) ) { $url = trim( $enclosure[0] ); } // Parse length and type. for ( $i = 1; $i <= 2; $i++ ) { if ( isset( $enclosure[ $i ] ) ) { if ( is_numeric( $enclosure[ $i ] ) ) { $length = trim( $enclosure[ $i ] ); } elseif ( in_array( $enclosure[ $i ], $mimes, true ) ) { $type = trim( $enclosure[ $i ] ); } } }
3 weeks ago
#13
Added a couple more notes re naming things.
Are you able to run
git rm tests/phpunit/tests/feed/feed.php tests/phpunit/tests/feed/rssEnclosure.php
too (please validate command, untested).
Sure.
✔️
Patch added