Steps to reproduce:
1. Using a text format with the "Caption images" filter enabled, add an image via the WYSIWYG using the image2 ckeditor plugin (aka default Image icon), and check the 'Caption' checkbox.
2. Add a caption in the WYSIWYG, so that when one views source in the WYSIWYG, a data-caption attribute is present on the image.
3. View page, observe image is displayed with caption as expected
4. Turn on theme debugging (sites/default/services.yml), clear/rebuild drupal caches.
5. View page, observe image has totally disappeared.

There's DOM manipulation code in Drupal\filter\Plugin\Filter\FilterCaption that assumes only one node will be present at a particular location in the tree, and replaces the original image with that one node. When theme debugging is on, whitespace text and comment nodes are also present, so the image is replaced by one of those.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mbaynton created an issue. See original summary.

mbaynton’s picture

Status: Active » Needs review
FileSize
1.58 KB
mbaynton’s picture

Worth mentioning, this would also affect any theme that overrode filter-caption.html.twig and output more than one element at the first level of the template.

mbaynton’s picture

For tests...how to make testbot use theme debugging?

mbaynton’s picture

Assigned: mbaynton » Unassigned
star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks!

@mbaynton probably something like this, this example is from \Drupal\system\Tests\Theme\TwigDebugMarkupTest.


    // Enable debug, rebuild the service container, and clear all caches.
    $parameters = $this->container->getParameter('twig.config');
    $parameters['debug'] = TRUE;
    $this->setContainerParameter('twig.config', $parameters);
    $this->rebuildContainer();
    $this->resetAll();
star-szr’s picture

Status: Needs work » Needs review
FileSize
1.57 KB
971 bytes

I wanted to test this manually and it didn't apply, here's a reroll with two minor CS fixes, see interdiff for those changes.

Wim Leers’s picture

Wow, great find! Test coverage still needed indeed.

star-szr’s picture

Status: Needs review » Needs work
mbaynton’s picture

Status: Needs work » Needs review
FileSize
3.38 KB
4.95 KB

@Cottser thanks for the snippet to configure twig debug; while writing a patch to TwigDebugMarkupTest I was reminded of this. Here's a test. Needed a new class as existing caption tests are in a subclass of Drupal\simpletest\KernelTestBase and the snippet for configuring debug needs WebTestBase.

No changes from 7 to the filter.

The last submitted patch, 10: 2567561-testonly.patch, failed testing.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Looks great! Just nitpicks :)

  1. +++ b/core/modules/filter/src/Tests/FilterTwigDebugTest.php
    @@ -0,0 +1,114 @@
    + * Tests Filter module filters with twig debugging on.
    ...
    +class FilterTwigDebugTest extends WebTestBase {
    

    This is all just for the "caption" filter. So let's rename it accordingly.

    None of the other filters will ever use the theme system, so none of them will actually ever be affected by this.

    So: FilterCaptionTwigDebugTest.

  2. +++ b/core/modules/filter/src/Tests/FilterTwigDebugTest.php
    @@ -0,0 +1,114 @@
    +  public static $modules = array('system', 'filter');
    ...
    +    $attached_library = array(
    +      'library' => array(
    

    array() -> []

  3. +++ b/core/modules/filter/src/Tests/FilterTwigDebugTest.php
    @@ -0,0 +1,114 @@
    +   * Ensures twig debugging is on in the current container.
    

    Enables Twig debugging.

  4. +++ b/core/modules/filter/src/Tests/FilterTwigDebugTest.php
    @@ -0,0 +1,114 @@
    +    if (! $parameters['debug']) {
    

    Space after exclamation mark

  5. +++ b/core/modules/filter/src/Tests/FilterTwigDebugTest.php
    @@ -0,0 +1,114 @@
    +   * Ensures twig debugging is off in the current container.
    

    Disables Twig debugging.

  6. +++ b/core/modules/filter/src/Tests/FilterTwigDebugTest.php
    @@ -0,0 +1,114 @@
    +   * Tests the caption filter with twig debugging on.
    

    s/twig/Twig/

Sagar Ramgade’s picture

Status: Needs work » Needs review
FileSize
4.88 KB

Patch attached with suggestions in #12

Status: Needs review » Needs work

The last submitted patch, 13: drupal-captioned_elements-2567561-13-D8.patch, failed testing.

Sagar Ramgade’s picture

Status: Needs work » Needs review
FileSize
4.91 KB

Oops file name and @file was left out, new patch attached

Wim Leers’s picture

Status: Needs review » Needs work

Could you please provide an interdiff? See https://www.drupal.org/documentation/git/interdiff.

Wim Leers’s picture

The embed D8 contrib module copied this faulty bit of code and therefore has the same bug: #2602316: Allow any HTML as content for replaceNodeContent.

Sagar Ramgade’s picture

Status: Needs work » Needs review
FileSize
4.91 KB
6.72 KB

Thanks, Patch with interdiff attached.

Status: Needs review » Needs work

The last submitted patch, 18: 2567561-18.patch, failed testing.

Wim Leers’s picture

Please use the -m option so we can see what has been renamed. This interdiff is useless unfortunately.

nils.destoop’s picture

@Sagar you still busy with this? Otherwise i'll create the interdiff. I need your patch for my project ;)

Wim Leers’s picture

@zuuperman: it's been more than a month, feel free to take over :)

Sagar Ramgade’s picture

Thank you go ahead.

gnuget’s picture

Status: Needs work » Needs review
FileSize
3.23 KB

Sorry for make the interdiff but I was working in the same bug in a different issue and I have fresh the code.

This interdiff is between #10 and #18.

David.

mbaynton’s picture

Thank you for finishing this up, @gnuget.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DX (Developer Experience)

Note that I also helped land #2602316: Allow any HTML as content for replaceNodeContent in the Embed module for Drupal 8. It duplicated this code, and hence it also includes this exact fix. That's further confirmation this is the right solution.

Three more nits that can be fixed on commit:

  1. +++ b/core/modules/filter/src/Tests/FilterCaptionTwigDebugTest.php
    @@ -0,0 +1,114 @@
    + * Tests Filter module filters with twig debugging on.
    

    Tests the caption filter with Twig debugging on.

  2. +++ b/core/modules/filter/src/Tests/FilterCaptionTwigDebugTest.php
    @@ -0,0 +1,114 @@
    +class FilterCaptionTwigDebugTest extends WebTestBase {
    +  /**
    

    Needs a newline between these.

  3. +++ b/core/modules/filter/src/Tests/FilterCaptionTwigDebugTest.php
    @@ -0,0 +1,114 @@
    +  public static $modules = array('system', 'filter');
    

    array() -> []

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3004bbb and pushed to 8.0.x and 8.1.x. Thanks!

diff --git a/core/modules/filter/src/Tests/FilterCaptionTwigDebugTest.php b/core/modules/filter/src/Tests/FilterCaptionTwigDebugTest.php
index 7a5361b..8b3d7ca 100644
--- a/core/modules/filter/src/Tests/FilterCaptionTwigDebugTest.php
+++ b/core/modules/filter/src/Tests/FilterCaptionTwigDebugTest.php
@@ -12,17 +12,18 @@
 use Drupal\filter\FilterPluginCollection;
 
 /**
- * Tests Filter module filters with twig debugging on.
+ * Tests the caption filter with Twig debugging on.
  *
  * @group filter
  */
 class FilterCaptionTwigDebugTest extends WebTestBase {
+
   /**
    * Modules to enable.
    *
    * @var array
    */
-  public static $modules = array('system', 'filter');
+  public static $modules = ['system', 'filter'];
 
   /**
    * @var \Drupal\filter\Plugin\FilterInterface[]
@@ -66,7 +67,7 @@ protected function setUp() {
     $this->debugOn();
 
     $manager = $this->container->get('plugin.manager.filter');
-    $bag = new FilterPluginCollection($manager, array());
+    $bag = new FilterPluginCollection($manager, []);
     $this->filters = $bag->getAll();
   }
 
@@ -91,12 +92,6 @@ function testCaptionFilter() {
       });
     };
 
-    $attached_library = [
-      'library' => [
-        'filter/caption',
-      ],
-    ];
-
     // No data-caption attribute.
     $input = '<img src="llama.jpg" />';
     $expected = $input;

Some minor tidy ups on commit.

  • alexpott committed e770c38 on 8.1.x
    Issue #2567561 by Sagar Ramgade, mbaynton, Cottser, gnuget, Wim Leers:...

  • alexpott committed 3004bbb on
    Issue #2567561 by Sagar Ramgade, mbaynton, Cottser, gnuget, Wim Leers:...
Wim Leers’s picture

Thanks Alex :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.