Opened 7 years ago
Last modified 6 years ago
#41905 new enhancement
Only loop over enclosure meta keys in rss_enclosure() and atom_enclosure()
Reported by: | birgire | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | 2.2 |
Component: | Feeds | Keywords: | has-patch has-unit-tests |
Focuses: | Cc: |
Description
Instead of getting all post meta keys for the current post and then filter out the enclosure
keys:
foreach ( (array) get_post_custom() as $key => $val ) { if ($key == 'enclosure') {
we could only get the ones we need with:
$meta_enclosures = get_post_meta( get_the_ID(), 'enclosure', false ); foreach ( (array) $meta_enclosures as $key => $val ){
where get_post_custom()
is a wrapper for get_post_meta( get_the_ID() )
.
Attachments (9)
Change History (24)
#3
@
7 years ago
There's already a core get_enclosed()
function, that I think we might use it here, but it also uses get_post_custom()
:
https://developer.wordpress.org/reference/functions/get_enclosed/
So I think we could make the above changes for get_enclosed()
.
#4
@
7 years ago
Had a better look and get_enclosed()
only returns the url part, leaving the length and type, so we can't use it here as a replacement.
#5
@
7 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
Added a test in 41905.2.patch.
#6
@
7 years ago
- Keywords needs-refresh needs-unit-tests added; has-unit-tests removed
Patch still applies cleanly to 4.9 RC2 at revision 42138. However, the unit test, "Tests_Feeds_Atom::test_atom_and_rss_enclosures" fails. I would also recommend splitting the RSS and Atom unit tests apart. There are two different files located in "tests/feed" for each feed type respectively. There may be some redundancy in these tests, but it is better to split them apart.
#7
@
7 years ago
- Keywords has-unit-tests added; needs-refresh needs-unit-tests removed
Thanks for the feedback @stevenkword
In 41905.3.diff I added
- a test for
rss_enclosure()
infeed/rss2.php
- and another one for
atom_enclosure()
underfeed/atom.php
.
I agree, it's sounds better to split it up, even though there's some duplication there.
All tests in rss2.php
seems to run successfully:
OK (12 tests, 170 assertions)
and same for atom.php
:
OK (3 tests, 95 assertions)
#8
@
7 years ago
Hi @birgire I hope you don't mind if I join in. I have modified tests to use a data provider.
phpunit --group=feed OK (18 tests, 268 assertions)
#10
@
7 years ago
@nateinaction you're surely welcome to join in and thanks for the patches.
I'm a fan of data providers, but the test in 41905.3.diff was adding multiple enclosures to a single post. The current data providers add a single enclosure. But I think it would be ideal if we could test both single and multiple enclosures, with the data providers.
The reason I added the cleanup before the assertion is from here:
https://core.trac.wordpress.org/ticket/42044#comment:1
We should also make sure to satisfy the WordPress code standard, I use the phpcs to check for that:
https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
#11
@
7 years ago
@birgire Thanks for the feedback!
This updated diff fixes the whitespace issues which conflicted with WP code standards. I have also fixed the data provider driven tests to align with with your suggestion:
I think it would be ideal if we could test both single and multiple enclosures, with the data providers.
Tested on 4.9RC2 at revision 42156:
phpunit --group=feed OK (18 tests, 268 assertions)
Please let me know what you think!
#12
@
7 years ago
@nateinaction thanks, exactly what I had in mind for the dataproviders ;-)
In 41905.7.diff I added doc description for the dataproviders and adjusted some minor things according to the documentation of the tests according the the WPCS.
So this looks good to me.
@stevenkword Is there anything else we need to take care of here?
#13
@
7 years ago
The patches and tests look good! Thanks! There are a few revisions I'd like to make to the PHPDoc, but nothing major. I'll follow-up with an updated patch ASAP.
#14
@
6 years ago
I refreshed the patch with 41905.8.diff as 41905.7.diff no longer applied cleanly to trunk.
@stevenkword do we need anything else here?
#15
@
6 years ago
The patch in 41905.9.diff adds the "no enclosures" case to the test data providers.
I will look into creating a test for this.