Convert [img] shortcodes back to tags/captions

[goldenapples]

See #20.

Introduces a method to the Img_Shortcode_Data_Migration class to convert
all the shortcodes added by this plugin back to the default WP format,
and a WP-CLI command to access it.

Still needs tests. Also there are some edge cases which break it (mostly dealing with caption widths), which should be identified and fixed in tests.

[davisshaver]

Ok, got a clean build going, going to revisit tomorrow and work on more test coverage for the reverse conversion.

[davisshaver]

Also there are some edge cases which break it (mostly dealing with caption widths), which should be identified and fixed in tests.

I haven't run into these yet. Any pointers @goldenapples? Also would you mind suggesting additional test coverage after looking at the ones I committed?

Also @danielbachhuber I don't think we are in habit of doing CLI test coverage, but let me know if you feel differently for this feature.

[goldenapples]

s/migrate/revert

[goldenapples]

Nice! I think you found all of the weird bugs I had left in there 😄 and fixed them. Thanks! The Cadillac tests are a nice touch.

I'm wondering if we need to account for cases where attributes in image tags are in a different order than is output by core. Currently the $image_tag_regex regex won't catch images in content if they're in a different format than the defaults from media_send_to_editor. So for example in our theme, if you ran wp image-shortcake revert and then tried to run wp image-shortcake migrate, you'd not get anything.

Also, WordPress will choke on any [caption] shortcodes without a width attribute, and usually fail to display them. Should we have a test in there making sure that we get a valid width attribute before reverting an [img] shortcode to [caption]?

I don't think we are in habit of doing CLI test coverage, but let me know if you feel differently for this feature.

We discussed this a while back, I think the decision was that WP-CLI is well-enough tested that we can consider it blackboxed. So long as the regex matches and the replacements long as the dat

[davisshaver]

@goldenapples re: [caption widths, the behavior is:
– If there's no width in the attribute, set the width to null.
– If the width is null, then the size is used to generate a width.
– If the size is null, then medium is used as the size.

I think we have adequate test coverage of this in the test file, but if you see a different test to add lemme know.

As for the mismatch between Image Shortcake and Core ordering of image attributes – I see two options.

1. Re-order Image Shortcake attribute ordering to be in parity with core.
2. Process the strings with a HTML parser instead of a regex.

Which do you prefer? or is there another option I'm missing?

cc @danielbachhuber

[goldenapples]

re: [caption widths,

Hmm. I tried running wp image-shortcake revert across some posts on my dev environment - which were exported from production recently. Here's the output of a couple:

On one post, missing attachment files mean no output at all in the replacements:

Image shortcode replacements for post 153190
============================================

-[img attachment="153522" align="aligncenter" size="full" alt="Clementa" /]
+
-[img attachment="153527" align="aligncenter" size="post-half-width" alt="Cynthia" /]
+
-[img attachment="153530" size="post-half-width" align="aligncenter" linkto="none" alt="Susie"]
+
-[img attachment="153539" align="aligncenter" size="post-half-width" alt="Ethel" /]
+
-[img attachment="153532" align="aligncenter" size="post-half-width" alt="Doctor" /]
+
-[img attachment="153533" align="aligncenter" size="post-half-width" alt="Tywanza" /]
+
-[img attachment="153542" align="aligncenter" size="post-half-width" alt="ht_emanuel_shooting_victim_07_daniel_simmons_jc_150618_4x3_992" /]
+
-[img attachment="153535" align="aligncenter" size="full" alt="Sharonda" credit="Facebook" /]
+
-[img attachment="153546" align="aligncenter" size="post-half-width" alt="Myra" /]
+

Where the attachment files exist, it works much better:

Image shortcode replacements for post 153683
============================================

-[img attachment="153689" align="alignnone" size="full" alt="mfg states" credit="Fusion, data via Berkeley Center on Reproductive Rights and Justice" /]
+<div class="image-no-caption alignnone" style="max-width: 100%; width:624px;"><img src="http://vip.local/wp-content/uploads/sites/2/2015/06/screen-shot-2015-06-19-at-11-18-45-am.png" srcset="http://vip.local/wp-content/uploads/sites/2/2015/06/screen-shot-2015-06-19-at-11-18-45-am.png 1200w, http://vip.local/wp-content/uploads/sites/2/2015/06/screen-shot-2015-06-19-at-11-18-45-am.png 960w, http://vip.local/wp-content/uploads/sites/2/2015/06/screen-shot-2015-06-19-at-11-18-45-am.png 740w, http://vip.local/wp-content/uploads/sites/2/2015/06/screen-shot-2015-06-19-at-11-18-45-am.png 600w, http://vip.local/wp-content/uploads/sites/2/2015/06/screen-shot-2015-06-19-at-11-18-45-am.png 480w, http://vip.local/wp-content/uploads/sites/2/2015/06/screen-shot-2015-06-19-at-11-18-45-am.png 320w, http://vip.local/wp-content/uploads/sites/2/2015/06/screen-shot-2015-06-19-at-11-18-45-am.png 150w" class="attachment-full" alt="mfg states" width="624" sizes="(min-device-resolution: 1.6 and max-width:40.063em) 200vw, (min-device-resolution: 1.6) 1248px, (max-width:40.063em) 100vw, 624px" height="394" /><span class="article-image-credit">Fusion, data via Berkeley Center on Reproductive Rights and Justice</span></div>

Trying to migrate again from that last one gives me nothing:

$ wp image-shortcake migrate 153683 --dry-run

Nothing to replace on post 153683. Skipping.

I'm not sure exactly how deep to get into this for this iteration. Using an HTML parser is an obvious solution and might be able to get around these problems. To a degree, though, these are edge case problems and it might be acceptable to leave them as open known issues until the plugin gets more use and we determine the use cases that are most important to support?

[davisshaver]

Ah, ok, the no attachment bug makes sense to me due to the isset on that parameter before the code that generates a fallback width.

You can see the test stub I started here for the missing attachment issue. https://github.com/fusioneng/image-shortcake/pull/31/files#diff-d6837b978cabb03629279a82d2b08c60R121

This is a tough one for me. Because WordPress is using src files, it seems nonsensical to reverse a shortcode to an img without a src. Maybe we just output the original shortcode in an HTML comment or something? Though after typing that even I am a bit embarrassed by how hacky it sounds...

[goldenapples]

Because WordPress is using src files, it seems nonsensical to reverse a shortcode to an img without a src.

Maybe we just output the original shortcode in an HTML comment or something?

I guess that could work. Would <img data-shortcode="[img%20attachment=%22156021%22%20align=%22alignnone%22%20/]"> be reasonable?

[davisshaver]

Hey @goldenapples, see here where @danielbachhuber recommends that we use the img tag, just w/o a src attribute.

If there's an attachment ID & a caption present, I'm still retaining that information in the element ID for the caption.

If there's an attachment ID & no caption, there is a loss of information. So yeah, I think a data-shortcode seems reasonable and doesn't cost much.

[goldenapples]

Storing just the attachment ID sounds good. This way all isn't lost. These are edge cases where if you run into these cases, you have bigger problems already.

The biggest case I wanted to avoid was where a shortcode would be replaced with an empty string, so that there was no possible recovering. Now we're always returning something, so if users find issues after running the CLI command, they can always reverse it using the CLI output.

[davisshaver]

@goldenapples Yes, in theory... I think that means we need to make sure that the migrate and revert commands can be run multiple times without loss of information though.

[goldenapples]

I think that means we need to make sure that the migrate and revert commands can be run multiple times without loss of information though.

Do you think that's even feasible, given that the output is filterable in so many ways?

[davisshaver]

Hm, I see what you mean re: filters.

It'd be nice if we could confirm on a vanilla WP install that such a loop is doable.

If we want to make the migration function as powerful as the callback in terms of customizability, then I suppose we'd need to add a filter or three to convert_img_tag_to_shortcode. This seems like it could come in a later release.

Agree with this? I guess next step would be adding a new site to my vip.local install and playing with the migrate/revert's.

[goldenapples]

It'd be nice if we could confirm on a vanilla WP install that such a loop is doable.

This seems like a simple test to set up, and yes, making sure this is possible without any other filters running would be a great thing to be sure of before shipping this feature.

we'd need to add a filter or three to convert_img_tag_to_shortcode.

👍 👍

[davisshaver]

Hm, actually, I think first I should add some more tests to test_img_tag_from_src. Doing that now.

[davisshaver]

Eh, maybe not, I didn't realize that test_img_tags_wrapped_in_links and test_replace_caption_shortcodes are covering this functionality, too.

[davisshaver]

Ok.. After playing with this on a new clean site, I think we should add some more test coverage to the regexes.

@goldenapples, what's the right way to test these given that they are private methods? Should we make them public or do something else?

[davisshaver]

Added some regex tests and temporarily changed the regexs to public while I work on getting them to pass.

[goldenapples]

what's the right way to test these given that they are private methods? Should we make them public or do something else?

You could also test the find_*_for_replacement functions and only look at the array keys of what it returns. But I don't see any reason the regexes shouldn't be exposed as public.

[davisshaver]

Ok, this is nearly there, but it's about closing time for me. The remaining test issue lies with test_replace_caption_shortcodes. I thought I was close a couple times, but alas I couldn't bring it home.

[davisshaver]

@goldenapples Hey - mind adding me back to this repo? The failing tests were bothering me so I finished them off this morning...

[goldenapples]

@davisshaver Thats freaking awesome! Doesn't look like I'm able to add collaborators to the repo, though. Might have to wait until @danielbachhuber gets back tomorrow for that?

[davisshaver]

👍

[davisshaver]

@danielbachhuber Can you add me so I can push the final commit?

[danielbachhuber]

Can you add me so I can push the final commit?

Done — apologies for the delay.

[davisshaver]

Thanks @danielbachhuber.

@goldenapples, passing this one back to you.