ResponsiveImagesCG / wp-tevko-responsive-images

Fully responsive image plugin for wordpress
449 stars 53 forks source link

Streamlining of 226 backcompat PR #239

Closed joemcgill closed 8 years ago

joemcgill commented 8 years ago

@jaspermdegroot I've been playing with a way of doing this without the use of a global variable and came up with a few ways of streamlining the process, I think.

First, I added a helper function, _tevkori_sizes_has_args() that caches the parsed $args param from tevkori_get_sizes() (when applicable). The same helper function can also be used to unset the cached $args value by passing in false.

In your shim function, I test to see if there is a cached $args param or a filter hooked to tevkori_image_sizes_args and bail early if not (which you were already doing).

When there is not a cached $args param but there is a filter, I recreate the default parsed $args from the original tevkori_get_sizes() instead of transforming the $sizes attribute into an array. Functionally, I believe you end up at the same place, but this was much easier for me to read.

I also added a unit test that makes sure that filters hooked into tevkori_image_sizes_args still fire when we call wp_calculate_image_sizes() directly.

One sidenote that I discovered in the process is that we have the second and third params of wp_calculate_image_sizes() documented as optional, but we never set default values for them. I'll open a separate tickets to address that here and in WP Core.

jaspermdegroot commented 8 years ago

@joemcgill

Using a static variable is indeed a better solution.

When there is not a cached $args param but there is a filter, I recreate the default parsed $args from the original tevkori_get_sizes() instead of transforming the $sizes attribute into an array. Functionally, I believe you end up at the same place, but this was much easier for me to read.

The thing is that I wanted to be sure that the width value in the $args array, matches with width value used in $sizes. image_downsize() always calls image_constrain_size_for_editor() while the new sizes functions don't. See https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/pull/219#issuecomment-153136070. I did merge your PR but I think this is something we still have to look into.

One sidenote that I discovered in the process is that we have the second and third params of wp_calculate_image_sizes() documented as optional, but we never set default values for them.

Good catch. I didn't see tickets for this yet. Let's make sure that we don't forget. While you're at it. The description of the $size argument says "Default: 'medium'", but it doesn't have a default / isn't optional.

Update: Found another mistake. In the the DocBlock of wp_get_attachment_image_sizes() the description of @return says "A 'srcset' value string or false." which should be "A valid source size value for use in a 'sizes' attribute or false.".

jaspermdegroot commented 8 years ago

I found more issues in the inline documentation so I opened a ticket for it: https://github.com/ResponsiveImagesCG/wp-tevko-responsive-images/issues/243

Let me know if you want me to do a PR and/or WP core patch for this or if you take care of it together with the missing default values in the code.

joemcgill commented 8 years ago

image_downsize() always calls image_constrain_size_for_editor() while the new sizes functions don't. See #219 (comment). I did merge your PR but I think this is something we still have to look into.

We discussed this in Slack and decided it would probably only be an issue in edge cases where someone was checking for a specific width size in their filter. Since this is a deprecated filter, we're going to let it ride, but we could alternately parse the width from the $sizes param if we get reports that this is an issue.