WordPress.org

Plugin Directory

jetpack

Opened 5 years ago

Closed 4 years ago

#1655 closed defect (duplicate)

Jetpack's Carousel has a logical issue in wp_get_attachment_link filter

Reported by: chrisbliss18 Owned by: tmoorewp
Priority: normal Severity: normal
Plugin: jetpack Keywords: carousel has-patch needs-testing
Cc: gaarai@…, richard@…

Description

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.

Attachments (1)

fix-jetpack-carousel-filter-logic.diff (1.3 KB) - added by chrisbliss18 5 years ago.
Add and use an in_gallery variable which is set by a low-priority post_gallery hook

Download all attachments as: .zip

Change History (6)

@chrisbliss185 years ago

Add and use an in_gallery variable which is set by a low-priority post_gallery hook

comment:1 @chrisbliss185 years ago

  • Keywords has-patch needs-testing added

comment:2 @chrisbliss185 years ago

  • Cc gaarai@… added

comment:3 @jeherve5 years ago

  • Owner changed from mdawaffe to tmoorewp

comment:4 @richardmtl4 years ago

  • Cc richard@… added

comment:5 @jeherve4 years ago

  • Resolution set to duplicate
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.