Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#24202 closed enhancement (fixed)

Self-explanatory argument values for new media functions

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: nacin's profile nacin
Milestone: 3.6 Priority: high
Severity: major Version: 3.6
Component: Media Keywords: dev-feedback has-patch
Focuses: Cc:

Description (last modified by SergeyBiryukov)

We've introduced a bunch of functions, some with a relatively long list of arguments, which accept booleans:

function get_content_media( $type, &$content, $html = true, $remove = false, $limit = 0 )
function get_embedded_media( $type, &$content, $remove = false, $limit = 0 )
function get_content_audio( &$content, $html = true, $remove = false )
function get_embedded_audio( &$content, $remove = false )
function get_content_video( &$content, $html = true, $remove = false )
function get_embedded_video( &$content, $remove = false )
function get_content_images( &$content, $html = true, $remove = false, $limit = 0 )
function get_content_image( &$content, $html = true, $remove = false )
function get_content_galleries( &$content, $html = true, $remove = false, $limit = 0 )
function get_post_galleries( $post_id = 0, $html = true )
function get_post_gallery( $post_id = 0, $html = true )
function get_content_chat( &$content, $remove = false )
function get_content_quote( &$content, $remove = false, $replace = '' )
function get_content_url( &$content, $remove = false )

I wonder if we can convert them to use arrays instead for future-proof changes, or at least switch from booleans to self-explanatory values, per our coding standards.

Otherwise, we might fall into a trap of submit_button(): #20492.

Attachments (12)

24202.diff (20.4 KB) - added by DrewAPicture 11 years ago.
First-pass.
24202.2.diff (25.8 KB) - added by DrewAPicture 11 years ago.
Second pass with docblocks
24202.3.diff (22.5 KB) - added by kovshenin 11 years ago.
24202.4.diff (22.9 KB) - added by kovshenin 11 years ago.
24202.5.diff (13.5 KB) - added by DrewAPicture 11 years ago.
Refreshed at r24580
24202.6.diff (15.6 KB) - added by nacin 11 years ago.
24202.7.diff (15.9 KB) - added by aaroncampbell 11 years ago.
24202.8.diff (16.2 KB) - added by aaroncampbell 11 years ago.
24202.9.diff (407 bytes) - added by aaroncampbell 11 years ago.
24202.10.diff (1.0 KB) - added by aaroncampbell 11 years ago.
24202.tests.diff (6.7 KB) - added by soulseekah 11 years ago.
Makes tests/media.php pass
24202.11.diff (439 bytes) - added by rodrigosprimo 11 years ago.
Fix typo in get_post_gallery_images()

Download all attachments as: .zip

Change History (53)

#1 @SergeyBiryukov
11 years ago

  • Keywords dev-feedback 2nd-opinion added

#2 @SergeyBiryukov
11 years ago

  • Description modified (diff)

#3 @SergeyBiryukov
11 years ago

Also, the $limit argument might be confusing.

In get_content_*() functions, it's the number of items to return:

 * @param int $limit Optional. The number of medias to return
...
function get_content_media( $type, &$content, $html = true, $remove = false, $limit = 0 )
...
 * @param int $limit Optional. The number of galleries to return
...
function get_embedded_media( $type, &$content, $remove = false, $limit = 0 )
...
 * @param int $limit Optional. The number of image srcs to return
...
function get_content_images( &$content, $html = true, $remove = false, $limit = 0 )
...
 * @param int $limit Optional. The number of galleries to return
...
function get_content_galleries( &$content, $html = true, $remove = false, $limit = 0 )

In get_the_post_format_media(), however, it's the number of items to remove from content:

 * @param int $limit Optional. The number of medias to remove if content is scanned.
...
function get_the_post_format_media( $type, &$post = null, $limit = 0 )

#4 @nacin
11 years ago

+1. Arrays can probably work. These mostly aren't template tags, after all.

@DrewAPicture
11 years ago

First-pass.

#5 @DrewAPicture
11 years ago

24202.diff takes a stab at a first pass converting args to arrays. I didn't really touch the docblocks yet.

$type    – becomes – 'type' (string)
$content – becomes – 'content' (string)
$html    – becomes – 'return_html' (bool)
$remove  – becomes – 'remove_from_content' (bool)
$limit   – becomes – 'media_count' (int)
$replace – becomes - 'replace_with' (string)

#6 @DrewAPicture
11 years ago

  • Cc xoodrew@… added
  • Keywords has-patch added

@DrewAPicture
11 years ago

Second pass with docblocks

#7 follow-up: @kovshenin
11 years ago

This is a little bit tricky. Content is passed around by reference in most cases, so that the function can change it. If we're going to use an array instead, we have to explicitly pass content to such functions by reference:

$media = get_content_media( array(
    'type'    => 'video',
    'content' => &$content,
) );

Otherwise the function will be changing its own copy of $content, and $remove would be pretty useless and confusing. We should consider leaving $content as a separate argument, passed by reference by default, and an $args array with everything else. Thoughts?

#8 in reply to: ↑ 7 @DrewAPicture
11 years ago

Replying to kovshenin:

We should consider leaving $content as a separate argument, passed by reference by default, and an $args array with everything else. Thoughts?

I think that makes good sense. I'd considered leaving the type param out of the array but it seemed to be a good fit when grouped with the other args.

Now that I look back on it, passing $content whether by reference or not -- as an arg -- seems odd. If it would simplify things leaving it out of the array, +1

@kovshenin
11 years ago

#9 @kovshenin
11 years ago

24202.3.diff builds off of Drew's patch:

  • $content is an argument of its own (again) passed by reference
  • Renamed the arguments back to their original values to avoid confusion (for now)
  • Looked through all usage of affected functions, .2 was missing a couple
  • Added more docblocks, rewrote $args docblocks to look more like what we use in register_post_type etc.
  • Removed trailing spaces, added trailing commas in multiline arrays

#10 @SergeyBiryukov
11 years ago

  • Keywords 2nd-opinion removed
  • Priority changed from normal to high
  • Severity changed from normal to major

@kovshenin
11 years ago

#11 @kovshenin
11 years ago

Refreshed in 24202.4.diff, hope it was worth the effort :) Touches the twenty themes too.

#12 @wonderboymusic
11 years ago

probably last call on this one, I have no desire to shepherd this - lots of changes

#13 @kovshenin
11 years ago

I'm happy to give this (yet) another go, but I'd like one of the core devs to tell me it won't be a waste of time, before I dig in :)

#14 @ocean90
11 years ago

  • Keywords needs-patch added; has-patch removed

A lot of functions where removed and the "remove" arg is gone, so this needs a refresh.

@DrewAPicture
11 years ago

Refreshed at r24580

#15 @DrewAPicture
11 years ago

  • Keywords has-patch added; needs-patch removed

24202.5.diff refreshes against current trunk (r24580).

@nacin
11 years ago

#16 follow-up: @nacin
11 years ago

I'm currently toying with 24202.6.diff:

  • Removes *all* limit arguments. For functions that return all by default, these make it more difficult for us to modify these functions in the future, as the order in which we extract/gather matters. Easier to just return everything and let them splice up the array. For the functions that return only one thing, a simple reset() is sufficient. Note that reset() returns false when the array is empty, which is fine (but not fully documented in @return at this time).
  • Renames get_content_url() to get_url_in_content(). markjaquith, koopersmith, and I came up with get_url_from_post_content() originally, and we can still experiment. But, this is still a step forward.
  • Removes get_content_media(), get_content_audio(), get_content_video(). Matching the [audio] and [video] shortcodes is not a priority, and it also misses things like the [embed] shortcode and oEmbed in general. Maybe these just needed better names, but it doesn't make sense to have these functions at this point.
  • get_embedded_media() becomes get_media_embedded_in_content(). While this also ignores oEmbed (not sure if oEmbed is more "content" media or "embedded" media — it's kind of both), the name helps make this more of a utility function for tag parsing — as does the reliance on a blob of content, and not a post ID. Support for a single line URL is removed (doesn't make sense to support this with the demise of PFUI, especially given that oEmbed — a similar concept — is unsupported). This function now searches for both audio and video, and get_content_audio() and get_content_video() is removed because they're not as useful anymore. get_embedded_media() could also return <iframe>, <embed>, and <object>, which could have been audio, video, or something else entirely. Let's keep the utility function, and go with something more generic.
  • get_attached_image_srcs() is also gone. Given get_attached_media(), there's not a huge need for this too.
  • get_content_images() is now get_images_in_content(). get_image_in_content() is now get_image_in_content(). Again — these are kept to content blobs only, not a post, to make them as generic helpers as possible. If we want to add more functionality in the future, these won't constrain us.
  • get_content_galleries() is gone. Galleries are intrinsic to a post. It is now merged with get_post_galleries(), which now has a filter (see #24309). get_post_gallery() returns the first gallery from get_post_galleries().
  • get_post_galleries_images() and get_post_gallery_images() are still there. I actually think that get_post_galleries_images() should return a flat array... As in, all images in all galleries. If you wanted individual per-gallery stuff, you can just call get_post_galleries() yourself.

By simplifying the function arguments — everything accepts $post or $content, and some also accept $html = true — I don't think we need to go with the array-style callback.

If we'd like to be compatible in the future, we should just make the second argument an array. But since it is currently a boolean, we can convert the function prototype to be an array later on.

#17 in reply to: ↑ 16 ; follow-up: @DrewAPicture
11 years ago

Replying to nacin:

I'd like to make a case for keeping the shortcode parsing logic in get_content_media(), maybe rolling it into something different like get_shortcodes_in_content(). It would be a good compliment to get_media_embedded_in_content() (media tags), and get_images_from_content() (images). Shortcodes, media tags, images.

There are many many more shortcodes out there than audio or video and I think keeping this logic would open some doors to some other interesting use cases, regardless of whether it's used by core or not.

Finally, I'd like to see us at least make that type array in get_embedded_media|get_media_embedded_in_content() filterable if we're going to remove the $type parameter.

#18 in reply to: ↑ 17 @nacin
11 years ago

Replying to DrewAPicture:

Replying to nacin:

I'd like to make a case for keeping the shortcode parsing logic in get_content_media(), maybe rolling it into something different like get_shortcodes_in_content(). It would be a good compliment to get_media_embedded_in_content() (media tags), and get_images_from_content() (images). Shortcodes, media tags, images.

There are many many more shortcodes out there than audio or video and I think keeping this logic would open some doors to some other interesting use cases, regardless of whether it's used by core or not.

In general I agree, but this wasn't really an API designed for media use, let alone generic use. It was designed around the fairly crazy needs for PFUI. If it's going to be in core as a nifty shortcode utility, we should try that in 3.7. We already added shortcode_exists() and has_shortcode() to 3.6, that's a good start in making this better for people.

Finally, I'd like to see us at least make that type array in get_embedded_media|get_media_embedded_in_content() filterable if we're going to remove the $type parameter.

Sure, it can gain $type = null as a second argument, with the ability to pass an array (intersected with those specified in the function).

#19 @aaroncampbell
11 years ago

I've been testing out 24202.6.diff and it seems to be working pretty well. 24202.7.diff updates the @uses in the docblock on twentythirteen_get_link_url() to s/get_content_url/get_url_in_content/.

#20 @aaroncampbell
11 years ago

24202.8.diff adds a $types argument to get_media_embedded_in_content() that intersects with the static list of allowed types.

#21 @aaroncampbell
11 years ago

Note: 24202.8.diff also fixed a docblock issue on get_media_embedded_in_content() that should go in even if the $types parameter doesn't

#22 @nacin
11 years ago

In 24682:

Simplify and reduce the new media/content extraction functions.

The URL extraction function is now get_url_in_content(). For more, see #24202.

Also adds filters to get_post_galleries() and get_post_gallery(). fixes #24309.

#23 @nacin
11 years ago

In 24683:

Move get_url_in_content() out of post-formats.php. see #24202.

#24 @aaroncampbell
11 years ago

24202.9.diff fixes just the phpdoc in get_media_embedded_in_content()

24202.10.diff also adds a $types argument to get_media_embedded_in_content() that intersects with the static list of allowed types.

#25 @nacin
11 years ago

In 24684:

Add types parameter to get_media_embedded_in_content(). props aaroncampbell. see #24202.

#26 @nacin
11 years ago

What's left here is I am not a huge fan of get_images_in_content(). While get_media_embedded_in_content() is fairly simple and straightforward, get_images_in_content() is pretty complicated and covers a bunch of different scenarios. On one hand, that's good. On the other, this feels like it has the potential to feel inferior in the future. Given galleries, featured images, and attached images, we already have quite a bit of association. I'm just not quite sure what this helps with right now.

Maybe if we dropped caption handling?

Dropping them entirely would also remove a few more $html = true parameters. (I'd like to rename these to $raw_html = true.)

#27 @aaroncampbell
11 years ago

I'm in favor of dropping get_images_in_content() and get_image_in_content(). I don't see enough benefit from them to justify the extra code.

If we do keep them I think we need to either drop support for caption or process all shortcodes (I'm sure plenty of other shortcodes add images), preferably the former. We also need to look at why it processes <a> tags (maybe I'm missing something here, but it seems pointless...maybe it's just leftover cruft from something that was PFUI specific).

#28 @nacin
11 years ago

Yeah, per IRC, they're gone.

#29 @nacin
11 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 24685:

Drop get_images_in_content() and get_image_in_content(), until recently get_content_image(s)(). fixes #24202.

#30 @nacin
11 years ago

Please, someone re-open if there is a particular issue with this refactoring.

#31 @lancewillett
11 years ago

In 24709:

Twenty Eleven: use the new URL extraction function name, see #24202 and r24682.

#32 @soulseekah
11 years ago

http://core.trac.wordpress.org/changeset/1260/tests has to be (at least partially) reverted since all those functions are now gone. Are more tests needed for the new functions introduced?

@soulseekah
11 years ago

Makes tests/media.php pass

#33 @ryan
11 years ago

In 1310/tests:

Remove tests for removed media functions.

Props soulseekah
see #24202

#34 @ryan
11 years ago

In 1311/tests:

s/get_content_url/get_url_in_content/

see #24202

#35 @ryan
11 years ago

In 1312/tests:

s/get_content_url/get_url_in_content/

see #24202

#36 @ryan
11 years ago

In 1313/tests:

Restore get_media_embedded_in_content() and test_post_gallery_images() and use their new function names. see #24202

@rodrigosprimo
11 years ago

Fix typo in get_post_gallery_images()

#37 @rodrigosprimo
11 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

See patch attached above for a fix in get_post_gallery_images(). Wrong variable name.

#38 @rodrigosprimo
11 years ago

  • Cc rodrigosprimo@… added

#39 @markjaquith
11 years ago

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

In 24837:

Fix a variable typo in get_post_gallery_images().

props rodrigosprimo. Fixes #24202 for trunk.

#40 @markjaquith
11 years ago

In 24838:

Fix a variable typo in get_post_gallery_images().

props rodrigosprimo. Fixes #24202 for 3.6.

#41 @ryan
11 years ago

In 1331/tests:

Update test_get_media_embedded_in_content() to reflect changes in get_media_embedded_in_content().

see #24202

Note: See TracTickets for help on using tickets.