WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 15 months 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:
PR Number:

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

See atom_enclosure() here and rss_enclosure() here.

Attachments (9)

41905.patch (3.9 KB) - added by birgire 2 years ago.
41905.2.patch (5.4 KB) - added by birgire 2 years ago.
41905.3.diff (6.6 KB) - added by birgire 2 years ago.
41905.4.patch (6.7 KB) - added by nateinaction 2 years ago.
41905.4.patch
41905.5.patch (6.7 KB) - added by nateinaction 2 years ago.
41905.6.patch (7.3 KB) - added by nateinaction 2 years ago.
41905.7.diff (8.0 KB) - added by birgire 2 years ago.
41905.8.diff (8.5 KB) - added by birgire 15 months ago.
41905.9.diff (8.8 KB) - added by birgire 15 months ago.

Download all attachments as: .zip

Change History (24)

@birgire
2 years ago

#1 @birgire
2 years ago

I will look into creating a test for this.

#2 @birgire
2 years ago

  • Keywords has-patch needs-unit-tests added

#3 @birgire
2 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 @birgire
2 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.

@birgire
2 years ago

#5 @birgire
2 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Added a test in 41905.2.patch.

#6 @stevenkword
2 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.

@birgire
2 years ago

#7 @birgire
2 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() 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)
Last edited 2 years ago by birgire (previous) (diff)

@nateinaction
2 years ago

41905.4.patch

#8 @nateinaction
2 years ago

Hi @birgire I hope you don't mind if I join in. I have cleaned up the tests a bit and modified them to use a data provider.

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

41905.4.patch

Version 0, edited 2 years ago by nateinaction (next)

#9 @nateinaction
2 years ago

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

41905.5.patch​

#10 @birgire
2 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 @nateinaction
2 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!

41905.6.patch

Last edited 2 years ago by nateinaction (previous) (diff)

@birgire
2 years ago

#12 @birgire
2 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 @stevenkword
2 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.

@birgire
15 months ago

#14 @birgire
15 months 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 @birgire
15 months ago

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

@birgire
15 months ago

Note: See TracTickets for help on using tickets.