Convert "Limit allowed HTML tags" input field to a textarea

Added Js modification to remove new line.

Some extra changes are:
1. This is showing error when node is null.

-        tag = node.tagName.toLowerCase();
+        tag = (node !== null) ? node.tagName.toLowerCase() : null;

2. Also can't able to access node.attributes when node is null. The very next line is finding .length. which needs to be passed empty string on null.

-        attributes = node.attributes;
+        attributes = (node !== null) ? node.attributes : '';

I tested the patch #6 and it works for me.

Patch also tested and working great, screenshot attached

I also tested patch #6 and it works for me. Steps taken to test:

• Spun up simplytest.me
• Tested allowed tags under configuration/text formats and editors/basic html with default values, an empty text area, and with a text area containing over 2048 characters
• Tested restricted html under configuration/text formats and editors/restricted html with default values, an empty text area, and with a text area containing over 2048 characters
• Verified that a) the data actually saved, b) the "The text format [name] has been updated" message displayed and c) that no errors were thrown.

In all case, tests passed.

+++ b/core/modules/filter/filter.filter_html.admin.js
@@ -98,6 +98,9 @@
+        $(this).val($(this).val().replace(/\n/g, ""));

I'm wondering if we should remove new lines also on save ? What if the person configuring does not have javascript ?

Patch tested and it was a JS error when textarea is empty and you leave the textarea context: Uncaught TypeError: Cannot read property 'length' of null

So I've added new check condition in miteshmap (#6) patch to solve that.

Testing #11:
With a text format that does not an editor, it works and does not throw the JS exception.
With a text format that uses the CKeditor,if I delete all the tags and save, the JS exception is gone now and the toolbar, when creating content that has a wysiwyg field, the toolbar is (correctly) empty (except for format and source buttons). However, when I go back to edit the configuration again, the tag list has re-populated to match the active toolbar. Only by editing the active toolbar (dragging the buttons off) can I make the tags not show back up in the allowed tags field when I reload the form.

This behavior did not happen with patch #6; in that case, on the configuration page, the displayed active toolbar remains the default (with buttons for bold, italic, etc) but the allowed tags field remains empty. On adding content, the editor shows the same format and source buttons only.

I have also reviewed the patch against Drupal git branch origin/8.0.x.

It is working:
1.) Change from text to textfield input.
2.) The 2048 restriction has been removed.

After review two potential size restraints based upon PHP and Database could occur:
1.) There is still a limit imposed by PHP's post_max_size configuration setting, which by default would allow 8 Megabytes or the php_memory_limit which could be less than 8 Megabytes in the event restricting or allowing a large amount of data to be entered into the database field.
http://php.net/manual/en/ini.core.php#ini.post-max-size

2.) In addition a user could potentially enter a value as large as the database type for the field which in MySQL is longblob. This could be an enormous amount of data.
http://dev.mysql.com/doc/refman/5.7/en/storage-requirements.html

This is good, but does anyone think that we should just impose a larger size for the textfield HTML restricted tags list so that it cannot be maxed out as described above? I understand that was not the original design of the task or patch, but it could mitigate the page from not submitting due to application error in addition to keeping the data manageable even for that one field.

Working on this issue as a part of DrupalCon Asia 2016 Code Sprint

I applied patch provided in #11. It fails to apply on latest 8.0.x branch. It needs to be recreated.

I applied the patch on Drupal 8.0.x-dev version on PHP5.5 and patch applied correctly and working fine.

#11 Works fine for me. Screenshot attached.

Okay. I applied the patch on old version. In new version, it works fine. Thank you.

LIVE FROM DRUPALCON ASIA WOOOOOOOO!!!

This was one of the patches we committed in live commit! Congrats to everyone!

xjm pointed out that swentel's comment wasn't yet addressed, so this may need some follow-up work done, but for now this looks like a totally great improvement over the status quo.

Because of the JS changes this one feels a bit risky to commit to 8.0.x, but let's go forward for 8.1.x. Yay!

Committed 5376602 and pushed to 8.1.x. Thanks!

Small followup for JS standards - #2672690: Fix JS standards in filter.filter_html.admin.js

I think we also need followups for #10 (swentel's comment about newlines) and #13:

This is good, but does anyone think that we should just impose a larger size for the textfield HTML restricted tags list so that it cannot be maxed out as described above? I understand that was not the original design of the task or patch, but it could mitigate the page from not submitting due to application error in addition to keeping the data manageable even for that one field.

Wish eslint was used before commit, especially since the issue wasn't tagged with JavaScript.

WOW! didn't know we created a follow up issue. Great!

+++ b/core/modules/filter/filter.filter_html.admin.js
@@ -98,6 +98,9 @@
+        $(this).val($(this).val().replace(/\n/g, ""));

#10++ and replacing \r\n

and #24++

Maybe revert it ?

Until a new patch or a follow up issue is created, I keep it Needs work :)

Let's leave this a fixed because we have a followup to address the issues and until eslint is run by testbot it is hard to enforce. Once it is - this will no longer happen.

Hmmm.... actually looking at the javascript more closely I think we have a problem.

+++ b/core/modules/filter/filter.filter_html.admin.js
@@ -98,6 +98,9 @@
+        $(this).val($(this).val().replace(/\n/g, ""));

Well missing handling of a carriage return ie. \r - see https://stackoverflow.com/questions/10805125/how-to-remove-all-line-brea...

I think that the whole line break question should be resolved in PHP and not javascript tbh :( And then we can open a followup to discuss the UX of removing them with JS.

Sadly I think this should be reverted too.

Here's an example of how to remove all the line breaks in a string in PHP from system_mail()

$message['subject'] .= str_replace(array("\r", "\n"), '', $subject);

Also looking at FilterHtml I'm not entirely sure that it even matters if new line characters are in this string - but that would need to be tested. So maybe we should just fix the js to handle new lines but not actually remove them. I'm not sure about this.

Should also remplace \r\n - unless windows has gotten smarter these days.

So actually the javascript changes were completely unnecessary after the changes in #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default. Basically

var allowedTags = setting.match(/(<[^>]+>)/g);

Strips all new lines outside of tags. It then uses the the browser to create elements of all the tags so spaces and returns don't (appear to) matter either - but maybe someone who worked on #2549077: Allow the "Limit allowed HTML tags" filter to also restrict HTML attributes, and only allow a small whitelist of attributes by default can confirm because the JS is quite complex.

The interdiff is correct but the patch was not right because of the revert...

Here's the patch - for some reason I can't upload patches today...

+++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -46,12 +46,10 @@ class FilterHtml extends FilterBase {
-      '#type' => 'textfield',
...
-      '#maxlength' => 2048,
...
-      '#size' => 250,

In fact, for this issue's purpose we only need these few lines.

Here's another problem of it didn't clean the data correctly ( Set & Get Raw data ) (that's why tests failed)

@droplet I disagree that we only need those lines - given that users can now add line breaks we should handle them in PHP so that it does not matter whether a user has JS enabled or not. With only those few lines and JS enabled line breaks would be removed but with JS disable line breaks would be saved. That is an inconsistency introduced by this patch and so should be solved here.

@alexpott,

I meant it's more than line break, you can adding whatever there now.

@droplet true tabs and all sorts... some of this was possible in the textfield days though too. I'd like to get this to the point where it does not matter if you have JS enabled or not you get the same result...

Atm without JS if you enter <em > it'll be saved like that - with JS it'll be saved as <em>. More problematically is <br/> with without JS is saved without modification but with JS it is saved (correctly) as <br> - but this is an existing issue and for followup.

#33: Confirmed that the JS already handles newlines just fine:

var tags = "<foo> <bar>\n<baz>\r\n<qux>".match(/(<[^>]+>)/g);
tags === ["<foo>", "<bar>", "<baz>", "<qux>"]

#40: Agreed. This needs to work correctly even when JS is off. You can easily prove that this is or isn't necessary, by updating the test coverage for \Drupal\filter\Plugin\Filter\FilterHtml::getHTMLRestrictions() to have an allowed_html setting that contains newlines and carriage returns.
#1936392: Configure CKEditor's "Advanced Content Filter" (ACF) to match Drupal's text filters settings is the issue that originally introduced ::getHTMLRestrictions(). Its test coverage lives at \Drupal\filter\Tests\FilterAPITest::testFilterFormatAPI(). Another possibility is expanding the test coverage in \Drupal\Tests\filter\Unit\FilterHtmlTest.

@droplet it was already possible to put garbage in here - this patch does not change that what it changes is the possibility of putting line breaks which you changed #38 from testing for some reason not explained in #44. And yes the current JS will clean up comments and the PHP will not that is what the follow up described in #42 will address.

@alexpott,

#44, I just wonder if the test cases didn't catch it correctly.

PHP will not that is what the follow up described in #42 will address.

I also agree to keep it same, no matter with/without JS. I just thought we can do them all at once in another issue.

@droplet but it is this issue that introduces the new line problem so we have to handle it here. Existing issues we don;t have to handle here.

Here's the unit test the @Wim Leers asked for.

I'm not argue with it anyway :). It's fine to me. Let's move it forward!!!

+++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -75,6 +73,12 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+      // new lines and double spaces so for consistency when javascript is

If I understand it correctly, here's not remove new lines.

** Free Talk, Don't Read: **
General speaking, you're right. Technically, nope. We still able to add line break in text input (OK. this is really not regular users can do). And if we only deal with the problem we introduce now, we should not take the space out. :P

@droplet because it leads to a better more elegant solution

+++ b/core/modules/filter/src/Plugin/Filter/FilterHtml.php
@@ -75,6 +73,12 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+      $configuration['settings']['allowed_html'] = preg_replace('/\s+/', ' ', $configuration['settings']['allowed_html']);

This removes new lines, tabs, spaces... any whitespace characters: \t\n\x0B\f\r

With this patch both the JS and PHP remove new lines.

Ahh right, Sorry! Needs some fresh air.

Patch #50 tested against 8.1.x. It applied cleanly and worked properly.

Hi @Swetha Yarla and @swetashahi,

When you assign issues to yourselves, can you please also add a comment explaining what you intend to do with the issue?

I am reviewing this at the drupalcon 2016 mentored sprints session.

I am reviewing this at drupalcon 2016.

My concern on #39, we can dicuss in public now: #2725255: Unfiltered data in "Allowed HTML tags"

I'm going to review this now...

Hi there,

I've tested this out on 8.4 in Simplytest.me. I:

- Added an
into the allowed list for filtered HTML with several newlines after it, then saved. The newlines are stripped out
- Added lots of tags (7872 characters in all). The field saved fine
- Tried to recreate the issue raised by @gadaniels72 above by creating a node using a WYSIWYG format, saving it then checking the allowed tags in the configuration, and I cannot replicate it.

This is a great change! I have totally hacked sites before to make exactly this improvement.

There seem to be about eight copies of the screenshot in the summary for some reason. Removing that and saving issue credit...

It looks like the original reasons this was reverted have been addressed, and as a bonus, it now has a JS maintainer's review and also test coverage. Thanks @jamesdesq for clearly documenting your manual testing.

There was one coding standard error:

FILE: ...maintainer/core/modules/filter/tests/src/Unit/FilterHtmlTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 95 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I fixed that on commit:

diff --git a/core/modules/filter/tests/src/Unit/FilterHtmlTest.php b/core/modules/filter/tests/src/Unit/FilterHtmlTest.php
index 8b8b8dfb38..664c2a3001 100644
--- a/core/modules/filter/tests/src/Unit/FilterHtmlTest.php
+++ b/core/modules/filter/tests/src/Unit/FilterHtmlTest.php
@@ -92,4 +92,5 @@ public function testSetConfiguration() {
     $filter = new FilterHtml($configuration, 'filter_html', ['provider' => 'test']);
     $this->assertSame('<a> <br> <p>', $filter->getConfiguration()['settings']['allowed_html']);
   }
+
 }

Committed to 8.4.x! I'm also so keen on this change that I backported it to the 8.3.x alpha as a nice usability improvement for the minor.

@xjm - I just wanted to say thanks for the acknowledgement above! It was the first time anyone had thanked me for working on a Drupal ticket, and it inspired me to go on and pick up some more issues.