Automattic/jetpack — pull request #101 by jeherve (2014-01-22 08:38:14+00:00)
Title: Carousel: logical issue in wp_get_attachment_link filter

One of my themes uses the `post_gallery` filter to modify the output so that it is more easily able to be responsive and handle more intricate styling. Thanks to the `jp_carousel_force_enable` filter, Carousel no longer silently fails in the theme; however, this exposes another issue. The logic used in the `add_data_to_images` function prevents this function from adding the needed modifications to the img tags due to the use of the `first_run` variable.

Here is what is happening:

* My theme adds a filter on `post_gallery` with a priority of 10.
* Carousel adds a filter on `post_gallery` with a priority of 1000, explicitly commenting that it wants to run after all other hooks on that filter.
* Carousel adds a filter on `wp_get_attachment_link` in order to modify the img tags in the gallery.
* Carousel sets the `first_run` variable to true.
* The gallery shortcode runs and the `post_gallery` filter is fired off.
* My theme function generates new gallery output, including images generated by calls to the `wp_get_attachment_link` function.
* On each call to `wp_get_attachment_link`, Carousel's `add_data_to_images` filter checks to see if it is in a gallery; however, it does this based upon the `first_run` variable, which is still set to true. Since this is the case, it fails to filter the gallery images.
* Carousel's `post_gallery` filter fires off, does it's enqueues, and then sets the `first_run` variable to false.
* Since the gallery is already generated, the gallery shortcode handler, returns the returned content and doesn't generate its own. This means that now that the `add_data_to_images` function, which can now work due to the changed `first_run` variable, will not get a chance to do its work.

The basic problem is that the `first_run` variable is being used a state variable to see if a filter is firing from inside of a gallery shortcode; however, this is not what the first_run variable does. Not only does it cause this specific problem where it causes failure when other code makes changes to the gallery output, it also creates situations where images generated by `wp_get_attachment_link` will have the additional modifications from `add_data_to_images`, even if they aren't actually in a gallery, just because they are after a gallery on the page.

In trying to come up with a solution, I first thought of creating an `in_gallery` variable that can be set to true on a very low-priority `post_gallery` hook and then set to false on a very high-priority `post_gallery` hook. However, this won't achieve the desired results as it only fixes the problem for my use case where the gallery output is being customized. Since these filters run before the gallery shortcode handler generates its gallery output, the variable would be set back to false on all default gallery output.

The reality is that there simply isn't a way to determine when a gallery is being rendered with absolute certainty. Any possible solution that I've come up with either has faults with false positives or faults with false negatives. The primary issue is that a low-priority hook on `post_gallery` works well to set the flag, but there isn't anything to reliably unset it.

Given that the code already has a bug in that the `add_data_to_images` function can filter image output beyond the scope of galleries, I decided to simply use the low-priority `post_gallery` hook to set the `in_gallery` variable to true and not have a condition to set the variable back to false. In other words, the supplied patch fixes the problem of breaking on sites where the gallery is modified in a way that does not affect Carousel while leaving the existing issue of possible non-gallery image modification in place.

Props @chrisbliss18

See original trac ticket:
https://plugins.trac.wordpress.org/ticket/1655