When you add 'class' as an allowed attribute in Basic HTML and save, it changes to class=""

Steps to reproduce:

  1. Edit the Basic HTML filter
  2. Add class to the allowed attributes of a tags
  3. Save
  4. class is changed to class=""

Screenshot of input before save:

Screenshot after save:

I note that the filter still works; I can still use the class attribute on a tags.

Original summary
---

Hi, sorry, my english is not perfect. Be indulgent.

After a fresh install of Drupal 8.0.5, i try to add some attributes to "a" html tags (title, class, id, rel, target) . This works, but for class attributes, it's save with additionnal string ="" (see attachment). It's added in Basic HTML text format. I do the same with Restricted HTML and it's ok.

CommentFileSizeAuthor
#32 interdiff_30-32.txt3.41 KBvsujeetkumar
#32 2693659-32.patch5.04 KBvsujeetkumar
#30 interdiff_28-30.txt1.37 KBvsujeetkumar
#30 2693659-30.patch1.46 KBvsujeetkumar
#28 interdiff-26_28-2693659.txt1.23 KBgauravvvv
#28 2693659-28.patch1.46 KBgauravvvv
#27 2693659-27.patch1.45 KBjofitz
#26 phpcs.png116.44 KByedhukrishnan.p@valuebound.com
#26 interdiff_23-26.txt377 bytesyedhukrishnan.p@valuebound.com
#26 2693659-26.patch811 bytesyedhukrishnan.p@valuebound.com
#23 after-patch.gif13.11 MBranjith_kumar_k_u
#23 2693659-23.patch646 bytesranjith_kumar_k_u
#20 Screenshot 2020-07-02 20.18.25.png26.03 KBpameeela
#20 Screenshot 2020-07-02 20.18.10.png25.72 KBpameeela
#15 2693659-15.patch780 byteskostyashupenko
#12 class_is_saved.png161.98 KBethomas08
#10 2693659-10.patch637 byteschiranjeeb2410
#9 2693659-9.patch780 byteschiranjeeb2410
#6 added_to_class-2693659-6.patch782 bytesoeklesund
09.png17.01 KByolker

Comments

yolker created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: filter.module » editor.module
Issue tags: +Needs steps to reproduce

Good catch!

RemyAroundTown’s picture

RemyAroundTown commented

I was able to reproduce this in 8.1. From the configuration screen for a text format, adding class as an attribute to any tag in the allowed tags list results in class="" being saved instead.

hershey.k’s picture

hershey.k commented
Assigned: Unassigned » hershey.k

Going to try and reproduce and attempt to fix the issue.

chiranjeeb2410’s picture

chiranjeeb2410 commented

@yolker, which file needs to be considered for this fix?

oeklesund’s picture

oeklesund commented
StatusFileSize
new added_to_class-2693659-6.patch782 bytes

This should prevent the issue.

oeklesund’s picture

oeklesund commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: added_to_class-2693659-6.patch, failed testing. View results

chiranjeeb2410’s picture

chiranjeeb2410 commented
Status: Needs work » Needs review
StatusFileSize
new 2693659-9.patch780 bytes

Patch uploaded against 8.1 core as mentioned by @RemyAroundTown in #3.
Review accordingly.

chiranjeeb2410’s picture

chiranjeeb2410 commented
StatusFileSize
new 2693659-10.patch637 bytes

Should do the trick. Please review.

ethomas08’s picture

ethomas08 commented

Testing on D8.6 and am not able to reproduce the bug. The code looks different so it looks like things must have been fixed at some point with updates. Screenshot attached to show that I could save a class added to the "a" in my filter settings.

ethomas08’s picture

ethomas08 commented
StatusFileSize
new class_is_saved.png161.98 KB

Screenshot png is called "class_is_saved"

alonaoneill’s picture

alonaoneill commented
Status: Needs review » Needs work
Issue tags: +Needs reroll
pcate’s picture

pcate commented

I used to have this issue, but like @ethomas08 I tested removing the =“” and reexporting config today with a site running 8.6, and the issue is no longer occurring.

The original config was exported when the site was running 8.4.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2693659-15.patch780 bytes
alonaoneill’s picture

alonaoneill commented

Patch applied on 8.8x!

cilefen’s picture

cilefen commented
Version: 8.0.5 » 8.8.x-dev
Component: editor.module » filter.module
Assigned: hershey.k » Unassigned

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

pameeela’s picture

pameeela commented
Title: Added ="" to class attributes in "Limit allowed HTML tags ..." » Empty quotes get added to class attribute in "Limit allowed HTML tags"
Issue summary: View changes
Issue tags: -Needs steps to reproduce
StatusFileSize
new Screenshot 2020-07-02 20.18.10.png25.72 KB
new Screenshot 2020-07-02 20.18.25.png26.03 KB

I can reproduce this so I updated IS with screenshots.

But as I note in the IS, the filter still works; I can still use the class attribute on a tags, so I am not sure that this needs to be fixed? It doesn't seem to be causing any problems.

pameeela’s picture

pameeela commented
Issue tags: +Bug Smash Initiative

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

ranjith_kumar_k_u’s picture

ranjith_kumar_k_u commented
StatusFileSize
new 2693659-23.patch646 bytes
new after-patch.gif13.11 MB

The last patch failed to apply on 9.2 ,re-rolled for 9.2
after patch

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

@ranjith_kumar_k_u, thanks for your interest in this issue. The latest patch didn't pass the coding standards check. Have a look at the instructions for running the coding standard checks locally so you can be sure the tests will run before uploading a patch. It also has the advantage of saving resources, including money, for the Drupal association. There are real costs for running the tests. And add an interdiff or a diff when doing a reroll. That will make it easier for the reviewer.

yedhukrishnan.p@valuebound.com’s picture

yedhukrishnan.p@valuebound.com commented
Status: Needs work » Needs review
StatusFileSize
new 2693659-26.patch811 bytes
new interdiff_23-26.txt377 bytes
new phpcs.png116.44 KB

I could not find any coding standard issues apart from the one that I have attached as screenshot image. Please review.

jofitz’s picture

jofitz
he/him
commented
Issue tags: -Needs reroll
StatusFileSize
new 2693659-27.patch1.45 KB

Include es6 changes in patch

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 2693659-28.patch1.46 KB
new interdiff-26_28-2693659.txt1.23 KB

Patch added, Interdiff attached for patch 26_28.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

The ES6 code fails our style guidelines:

/var/www/html/core/modules/filter/filter.filter_html.admin.es6.js
  341:13  error  All 'var' declarations must be at the top of the function scope  vars-on-top
  341:13  error  Unexpected var, use let or const instead                         no-var
  343:41  error  Unexpected string concatenation                                  prefer-template
vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new 2693659-30.patch1.46 KB
new interdiff_28-30.txt1.37 KB

Fixed the ES6 code fails, Please have a look.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Thanks. I guess this needs a functional JavaScript test to ensure we don't break it again in the future. There is already a JS test for the HTML filter so it should be possible to extend that.

vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new 2693659-32.patch5.04 KB
new interdiff_30-32.txt3.41 KB

Added Tests, Please have a look.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented

Believe this is a duplicate of https://www.drupal.org/project/drupal/issues/2710427#comment-14627888 or also being solved over there. Could someone confirm?

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented
Status: Needs review » Closed (duplicate)

I agree with @smustgrave, this is a duplicate of #2710427: Broken "Allowed Tags" updating: after all values for an attribute are allowed, it should not be overridden to allow only certain attribute values.

I would normally favour the issue that was opened earlier (this one) but in this case the other ticket has a more fleshed out issue summary and a good deal more recent discussion, so closing this ticket instead.

We have asked in #bugsmash for credit to be transferred from this ticket to that one.

quietone’s picture

quietone commented

Moving credit