archetyped / simple-lightbox

The highly customizable lightbox for WordPress
http://archetyped.com/tools/simple-lightbox/
GNU General Public License v2.0
74 stars 33 forks source link

Issue: Weaver Show Posts plugin breaks Simple Lightbox #419

Closed iNed closed 9 years ago

iNed commented 9 years ago

Description of Problem

When a Weaver Show Posts shortcode is included on the page/post, Simple Lightbox no longer functions (i.e. clicking a linked image just opens a new page showing the image).

I reported this problem to the Weaver Show Posts developer here: http://forum.weavertheme.com/discussion/12625/weaver-show-posts-breaks-simple-lightbox-plugin He examined my test site, built purely to resolve this conflict, and said he thought it was a Simple Lightbox problem.

Details

I have created a bare-bones test case (new/fresh install of all components) that demonstrates the problem:

http://prettymuch.biz/sites/lightbox_test/without-show-posts/ (Lightbox functionality works correctly) The code on this page is simply: [gallery link="file" ids="8,7,6,5,4,9"]

http://prettymuch.biz/sites/lightbox_test/with-show-posts/ (No lightbox functionality) The code on this page is simply: [gallery link="file" ids="8,7,6,5,4,9"] [show_posts filter=default]

The ONLY plugins installed are: Simple Lightbox - Version 2.5.0 (latest version) Weaver Show Posts - Version 1.3 (latest version)

The theme in use is the default Twenty Fifteen theme that comes with WordPress.

I created this test site purely for troubleshooting this conflict, so I would be happy to give full admin access (via PM) if need be.

What I noticed on the page where SLB does not work is that: 1) this parameter set is missing from the links (data-slb-active="1" data-slb-asset="1500893077" data-slb-internal="0" data-slb-group="10") 2) the entire slb_footer section is missing

archetyped commented 9 years ago

Hi, thanks for the awesome detailed report, it really is appreciated. I took a look at the Show Posts plugin and it looks like the issue is being caused when it calls the_content() as part of the shortcode's execution.

Shortcodes in posts/pages are processed as part of the the_content filter, which is called by the_content(). When the_content() is called inside an already-running the_content filter, the existing filter's process is reset. As a result, other processes hooked into the_content that run after the Show Posts shortcode are not executed. This affects not only SLB, but any other code that hooks into the_content.

To avoid this issue, the Show Posts shortcode should be backing up the state of the the_content filter that is already in progress before calling the nested the_content() and restore it once the shortcode has finished its work.

I'm sorry that you're getting the runaround here, but you can direct the Show Posts developers to this ticket and hopefully they'll be able to resolve the shortcode's issue quickly. You can also direct them to this pull request of another plugin that previously had the same nested the_content() issue as theirs with further details on the issue.

iNed commented 9 years ago

Thank you so much, archetyped, for your thorough analysis and your quick response. I will forward your response to the Weaver Show Posts developer. Thank you again.

weavertheme commented 9 years ago

I put in a "fix" for Weaver Xtreme and Weaver Show Posts (it was required both places.)

I'm not convinced that this really was a Weaver issue, but an issue with how Simple Lightbox does things. My fixes work with Weave Xtreme + Weaver Show Posts, but do not work with any of the Twenty-XXX themes and Lightbox. Of course, Show Posts works fine with the Twenty-XXX theme without Lightbox, and it is only Lightbox that is broken

Weaver Show Posts knows how to use the default post content display function (simply content.php using the standard WP get_template function found in about 50% of all WP themes) for many many themes. Getting them all to work with both Show Posts and Simple LIghtbox would require fixing ALL of their content.php files, and that just isn't right.

So - if a plugin breaks in a Twenty-XXX theme, I would place the burden of correct operation on the plugin, in this case, Simple LIghtbox.

I didn't look at Lightbox code, but it sort of seems to me that the need for the deeper the_content filtering might have to do with whether or not to emit the required JavaScript at the end of the generated HTML for the post/page. (Or perhaps changing the priority of the Lightbox the_content filter so it fires last?)

If that is the case, the way I do that is to set a $GLOBALS value (that my shortcode was indeed used in the content), and then use a footer action to check the $GLOBALS and emit the script.

But I really don't think any plugin should require other plugins to add a really kludgy on-the-fly fix to the 'the_content' filter stack to make them work with it.

archetyped commented 9 years ago

Hi, did you look at the look at the aforementioned pull request that has more details or at WordPress' own apply_filters() function? It's not a matter of only SLB being broken by Show Posts, but any code that is hooked into the_content after Weaver Show Posts' shortcode will not run.

Simply put: the_content() is not designed to be nested within another the_content() call. Therefore, if this is done, there also needs to be safeguards in place to ensure that it doesn't muck up WordPress' normal operation.

If you can explain why this is not a problem, I would be glad to hear your thoughts on what you recommend other plugins do when Weaver Show Posts ends the filtering process prematurely.

iNed commented 9 years ago

I appreciate that both of you (archetyped & weaver) are working together on a resolution to this.

I have relied on SLB & Weaver themes for years now, because 1) they just work right, with no fuss 2) if they don't, I've observed how you both give timely and cogent support.

I'm just a low level, random web designer guy, but I'm gratified to see that you have taken my problem report seriously. Thank you.

weavertheme commented 9 years ago

Well, Show Posts is in use on over 8,000 sites now, and as far as I know, SLB is the only plugin that is incompatible. We know about plenty of other plugins that would be filtering the_content (e.g., similar posts, social links, advertising, etc), and none of them have ever had any issues.

I wouldn't imagine that ALL plugins have been used with Show Posts, but the sample size is pretty big.

weavertheme commented 9 years ago

I figured out how to move the patch to a higher level - and the Show Posts will now work even with other themes with their content.php files.

Also found some bug reports in WP trac that suggest that failing recursive calls the the_content are considered by at least some to be a bug. I couldn't really tell if/when that might have been fixed, but any fixes were probably not backward compatible, so the fixes never made it in.

So I would be firmly in the camp that the_content should be fully recursive, including the filter. That is totally stupid that it wouldn't be.

But the patch in the above pull request seems to work - at least for now. But this is still the first instance of finding an incompatible plugin - and the code used by [show_posts] actually goes way back to 2010 and Weaver I and Weaver II and probably has tens of thousands use cases.

archetyped commented 9 years ago

I am more than glad to discuss this further, but unfortunately there's not much to discuss if you aren't willing to look into how WordPress works.

Here's a simple test you can drop in anywhere (such as your theme's functions.php file) to see the effect Show Posts is having:

function after_shortcodes($content) {
    $content .= "<br> This text is inserted after shortcodes have run.  Things are swell.";
    return $content;
}

add_filter('the_content', 'after_shortcodes', 12);

The after_shortcodes() function runs after shortcodes (including Show Posts) have run. When Show Posts is not enabled (or the shortcode is not used in the post), then the text is displayed. When Show posts is enabled, the text is not displayed in the main post due to the the_content filter process being prematurely cut short by Show Posts' nested the_content() call.

Let me know if you have any questions.

weavertheme commented 9 years ago

I really appreciate you help on this - really.

After thinking about my initial reaction to your explanation of the problem, I came to the realization that I literally could not believe what you described could be real. Seriously.

While the code you provided seems to fix the problem, the fact that it is necessary, and that whoever came up with it originally had to go deep into the core WP code and understand some very obscure stuff, indicates to me a truly flawed implementation by WP at some point. In my 40 years of programming and teaching software engineering, this is seriously one of the best (worst?) examples I've ever seen of how not to do it. I really had trouble believing that this bit of non-functionality could be true.

And the fix is a perfect example of an ugly kludge that doesn't belong in anyone's code. A truly perfect example for a textbook. But it is clearly necessary. Unfortunately, if WP ever does make the_content and the_content filter support recursion as they should have all along, this fix might then break things. Ugly ugly ugly.

It is just so obvious to me that one would want posts within a page, or even posts within a post, and that shortcodes are the obvious way to implement that. So the whole point of how the_content logically works (or even using get_the_content() and using the_content filter as logic would dictate) should support that. There is the wp_reset_query() function to allow recursion on the loop, so why not the same support for the content filter?

I guess it is even worse than that if the whole filter processor doesn't allow recursion. Just plain bad design in my book. I guess most of the time it doesn't matter, but this example of Show Posts really uncovers a nasty den of snakes, as far as I'm concerned.