Only loop over enclosure meta keys in rss_enclosure() and atom_enclosure()

[birgire]

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() ).

See atom_enclosure() ​here and rss_enclosure() ​here.

[birgire]

I will look into creating a test for this.

[birgire]

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().

[birgire]

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.

[birgire]

Added a test in 41905.2.patch​.

[stevenkword]

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.

[birgire]

Thanks for the feedback @stevenkword

In 41905.3.diff​ I added

• a test for rss_enclosure() in feed/rss2.php
• and another one for atom_enclosure() under feed/atom.php.

I agree, it sounds better to split it up, even though there're some duplications 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)

[nateinaction]

41905.4.patch

[nateinaction]

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)

41905.4.patch

[nateinaction]

Updated to use expectOutputString() in tests as @birgire did in 41905.3

41905.5.patch​

[birgire]

@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

[nateinaction]

@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.

And all tests pass:

phpunit --group=feed
OK (18 tests, 268 assertions)

Please let me know what you think!

41905.6.patch​

[birgire]

@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?

[stevenkword]

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.

[birgire]

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?

[birgire]

The patch in 41905.9.diff​ adds the "no enclosures" case to the test data providers.