Self-explanatory argument values for new media functions

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

[SergeyBiryukov]

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 )

[nacin]

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

[DrewAPicture]

First-pass.

[DrewAPicture]

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)

[DrewAPicture]

Second pass with docblocks

[kovshenin]

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?

[DrewAPicture]

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]

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

[kovshenin]

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

[wonderboymusic]

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

[kovshenin]

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 :)

[ocean90]

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

[DrewAPicture]

Refreshed at r24580

[DrewAPicture]

24202.5.diff​ refreshes against current trunk (r24580).

[nacin]

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.

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

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.

[nacin]

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

[aaroncampbell]

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

[aaroncampbell]

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

[aaroncampbell]

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

[nacin]

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.

[nacin]

In 24683:

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

[aaroncampbell]

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.

[nacin]

In 24684:

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

[nacin]

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

[aaroncampbell]

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

[nacin]

Yeah, ​per IRC, they're gone.

[nacin]

In 24685:

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

[nacin]

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

[lancewillett]

In 24709:

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

[soulseekah]

​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]

Makes tests/media.php pass

[ryan]

In 1310/tests:

Remove tests for removed media functions.

Props soulseekah
see #24202

[ryan]

In 1311/tests:

s/get_content_url/get_url_in_content/

see #24202

[ryan]

In 1312/tests:

s/get_content_url/get_url_in_content/

see #24202

[ryan]

In 1313/tests:

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

[rodrigosprimo]

Fix typo in get_post_gallery_images()

[rodrigosprimo]

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

[markjaquith]

In 24837:

Fix a variable typo in get_post_gallery_images().

props rodrigosprimo. Fixes #24202 for trunk.

[markjaquith]

In 24838:

Fix a variable typo in get_post_gallery_images().

props rodrigosprimo. Fixes #24202 for 3.6.

[ryan]

In 1331/tests:

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

see #24202