[wp-meta] [Making WordPress.org] #5746: wporg-glossary plugin doesn't correctly handle being run twice over the same content

[Making WordPress.org]

#5746: wporg-glossary plugin doesn't correctly handle being run twice over the
same content
--------------------------------------+--------------------
 Reporter:  pbiron                    |      Owner:  (none)
     Type:  defect                    |     Status:  new
 Priority:  normal                    |  Milestone:
Component:  Make (Get Involved) / P2  |   Keywords:
--------------------------------------+--------------------
 In `Glossary_Handler::glossary_links()`, there is logic that tries to skip
 adding links in content...but that logic is incorrect.

 In particular,

 {{{#!php
 // Skip the Glossary item container span, for when the_content is run over
 the_content.
 if ( $matches && 'span' == $matches[0] && false !== strpos( $element,
 "class='glossary-item-container'" ) ) {
         if ( ! $is_end_tag ) {
                 array_unshift( $inside_block, $matches[0] );
         } elseif ( $inside_block && $matches[0] === $inside_block[0] ) {
                 array_shift( $inside_block );
         }

         continue;
 }
 }}}

 Unfortunately, the `false !== strpos( $element, "class='glossary-item-
 container'" )` clause in the conditional will only ever be true on the
 opening tag.  Because of that, the `span` that gets pushed onto
 `$inside_block` will never get popped.

 The result is that once it sees an opening span with that class,
 `$inside_block` will never be empty again (unless there is some seriously
 unbalanced markup involving any of the `$ignore_elements`) and from that
 point on no other occurrences of glossary terms will get the hovercard
 markup added to them.

 I discovered this while investigating possibly using this plugin on a
 client site.  It's unclear how likely this issue is to surface on .org
 content.

 Here's a simple example to illustrate the bug (abstracted from that client
 site).  Suppose the glossary contains `term1` and `term2`, and a post with
 the following content:

 {{{
 [some_shortcode]this is term1[/some_shortcode]
 <p>a paragraph with term2</p>
 }}}

 Further suppose the plugin that registers [some_shortcode], does something
 like:

 {{{#!php
 add_shortcode( 'some_shortcode', function( $atts, $content ) {
         return apply_filters( 'the_content', $content );
 } );
 }}}

 So, what `Glossary_Handler::glossary_links()` sees the 2nd time is:

 {{{
 <span tabindex="0" class="glossary-item-container">some term<span class
 ="glossary-item-hidden-content"><span class="glossary-item-
 header">Administrator</span> <span class="glossary-item-description">some
 definition</span></span></span></span>
 <p>a paragraph with term2</p>
 }}}

 The result is that `term2` does **not** end up with a hovercard.

 I'm not sure what the easiest/best way to fix this is.  Conceptually, what
 needs to happen is once a `<span class='glossary-item-container'>` is
 found, it needs to start pushing/popping tags onto **another** stack so
 that it can detect when it's seen the associated closing `</span>`, in
 which case it should pop the span off of `$inside_block`.  But that can
 get messy real quick, especially if it needs to account for unbalanced
 tags in the glossary item description.

-- 
Ticket URL: <https://meta.trac.wordpress.org/ticket/5746>
Making WordPress.org <https://meta.trac.wordpress.org/>
Making WordPress.org