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.
Comment | File | Size | Author |
---|---|---|---|
#24 | interdiff-2567561-10-28-24.txt | 3.23 KB | gnuget |
#18 | interdiff-2567561-10-18.txt | 6.72 KB | Sagar Ramgade |
#18 | 2567561-18.patch | 4.91 KB | Sagar Ramgade |
#15 | drupal-captioned_elements-2567561-15-D8.patch | 4.91 KB | Sagar Ramgade |
#13 | drupal-captioned_elements-2567561-13-D8.patch | 4.88 KB | Sagar Ramgade |
Comments
Comment #2
mbayntonComment #3
mbayntonWorth 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.
Comment #4
mbayntonFor tests...how to make testbot use theme debugging?
Comment #5
mbayntonComment #6
star-szrThanks!
@mbaynton probably something like this, this example is from \Drupal\system\Tests\Theme\TwigDebugMarkupTest.
Comment #7
star-szrI wanted to test this manually and it didn't apply, here's a reroll with two minor CS fixes, see interdiff for those changes.
Comment #8
Wim LeersWow, great find! Test coverage still needed indeed.
Comment #9
star-szrComment #10
mbaynton@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 needsWebTestBase
.No changes from 7 to the filter.
Comment #12
Wim LeersLooks great! Just nitpicks :)
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
.array()
->[]
Enables Twig debugging.
Space after exclamation mark
Disables Twig debugging.
s/twig/Twig/
Comment #13
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedPatch attached with suggestions in #12
Comment #15
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedOops file name and @file was left out, new patch attached
Comment #16
Wim LeersCould you please provide an interdiff? See https://www.drupal.org/documentation/git/interdiff.
Comment #17
Wim LeersThe
embed
D8 contrib module copied this faulty bit of code and therefore has the same bug: #2602316: Allow any HTML as content for replaceNodeContent.Comment #18
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedThanks, Patch with interdiff attached.
Comment #20
Wim LeersPlease use the
-m
option so we can see what has been renamed. This interdiff is useless unfortunately.Comment #21
nils.destoop CreditAttribution: nils.destoop as a volunteer and at Wunder commented@Sagar you still busy with this? Otherwise i'll create the interdiff. I need your patch for my project ;)
Comment #22
Wim Leers@zuuperman: it's been more than a month, feel free to take over :)
Comment #23
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedThank you go ahead.
Comment #24
gnugetSorry 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.
Comment #25
mbayntonThank you for finishing this up, @gnuget.
Comment #26
Wim LeersNote 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:
Tests the caption filter with Twig debugging on.
Needs a newline between these.
array()
->[]
Comment #27
alexpottCommitted 3004bbb and pushed to 8.0.x and 8.1.x. Thanks!
Some minor tidy ups on commit.
Comment #30
Wim LeersThanks Alex :)