#allowed_formats property for #type 'text_format' to filter available formats

Although I think introducing the #formats property is great, it might be better to name it #allowed_formats. This will help avoid confusion with the #format property, and make it easier to understand what the property is for.

There is Drupal 7-targeted contrib code in #1295248: Allow per-field-instance configuration of allowed formats that I welcome and encourage people to steal for Drupal 8. This is an important feature for any moderately complex Drupal site.

Let's keep this issue focused on the API side of the functionality. A possible UI for this should be tackled in a separate follow-up issue.

I don't see any problems with the feature request itself, but I do have some concerns the implementation will have to address:

1. Would it make more sense to negate it? I.e., #disallowed_formats?

   We should investigate what the actual real world use-cases of this functionality are.

   Background: From a code and data handling perspective, I'm questioning whether it wouldn't it be easier for modules to only specify what they do not want. Because, precisely specifying what you want means that you have to keep track of all available formats on your own, and implement your own logic to handle the cases of when a new format has been created, or an existing is deleted.

2. The implementation needs to create/design a new (test) expectation for the special case when all available formats are nuked by the #allowed_formats override and no format is left.

   This also includes the separate case when the currently assigned #format (for some existing text) is no longer available.

   The current widget handles this already, but exclusively on the assumption that these cases mean that the user does not have access to the assigned #format. Reducing the formats as proposed here has nothing to do with user access privileges to formats and the security considerations involved.

   Thus, we not only need expectations, but we also need to implement an appropriate behavior for those cases.

Use case:

- Forum post nodes and comments should allow only a super-restricted set of HTML, with Markdown support. That is true for all users regardless of role.

- Page nodes should allow a somewhat more permissive set of tags, but still not img or table. However, users in the Privileged Admin role should also have a second format available that allows img, table, etc.

- Major Event nodes allow the same two formats as Page noes, with the same role-based restriction, but it has WYSIWYG enabled for both formats.

- Major Event also has an "about the location" field that only allows certain HTML tags. This set of allowed tags is different than what forum posts or pages/events allow.

Note: This is not a contrived example. I originally wrote filterbynodetype in Drupal 5 and Drupal 6 because I had very nearly the use case above.

Cross-linking this issue with #784672: Allow text field to enforce a specific text format, which has some patches and previous discussion already.

In theory, it would make sense to create the API first (in this issue) and then consider using it later (in the other issue) - currently the code in the other issue is a bit of a mix of both. So we could postpone that issue on this one, except all the code is currently over there.

Doing some research in the issue queue, there are a lot more use cases where this functionality is badly needed. With this minor API change, all these use cases could be implemented in contrib can be done without hacky form manipulations.

Here's a patch with test covering the essential cases and a few minor modifications to fix warnings. As sun mentioned in #3, this patch focuses only on the API-level functionality.

Example usage:

// Limit the allowed text format to "full_html".
$form['some_field'] = array(
  '#type' => 'text_format',
  '#format' => 'full_html',
  '#allowed_formats' => array('full_html'),
);

The logic inside filter_process_format() has also been updated to function similarly when the default format is not in the allowed formats as when the default format has been deleted.

Sorry for the whitespace stuff, I hadn't set things up properly in a new environment. Here's the patch rerolled with both changes.

Reroll

I do not plan to work on this personally, but it shouldn't be too hard, actually

Yeah

In general I totally agree that the basic usecase is to limit the available formats, instead of disallow what you don't want.
While people want to have maybe something more HTML-ish/BB-ish/markdown-ish in a forum, they do not need that in a contact form.
One an architectual perspective it could help a module like better_formats to have the separate process method to deal with the #allowed_formats but given
the amount of logic in the lower part of the function, this does not seem to be feasible.

1. +++ b/core/modules/filter/filter.module
   @@ -719,9 +723,28 @@ function filter_process_format($element) {
   +      // If the user does not have access to any of the allowed formats, set
   +      // the default format to NULL.
   +      $element['#format'] = NULL;
   

   Does that automatically falls back to the fallback format again, or in other words the comment should not say what the code is doing.
   Oh I see you commented the same, removed the other ones which aren't really helpful.

2. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,267 @@
   +    $this->drupalLogin($this->adminUser);
   ...
   +    $this->doFilterFormTestAsAdmin();
   ...
   +    $this->drupalLogin($this->webUser);
   ...
   +    $this->doFilterFormTestAsNonAdmin();
   

   It is a bit odd to have the drupalLogin calls outside of the methods which talk about working "asAdmin"/"asNonAdmin" but I cannot think about a better way without hardcoding the path in the actual test method.

3. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,267 @@
   +  protected function assertNoSelect($id) {
   ...
   +  protected function assertOptions($id, array $expected_options, $selected) {
   ...
   +  protected function assertRequiredSelect($id, array $options) {
   ...
   +  protected function assertDisabledTextarea($id) {
   

   We could think about moving up assertRequiredSelect and assertOptions, they seem to be quite useful.

1. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.php
   @@ -32,36 +32,6 @@ function setUp() {
   @@ -72,13 +42,17 @@ function testCheckMarkup() {
   
   @@ -72,13 +42,17 @@ function testCheckMarkup() {
        $expected_filtered_text = "Text with evil content and a URL: <a href=\"http://drupal.org\">http://drupal.org</a>!";
        $expected_filter_text_without_html_generators = "Text with evil content and a URL: http://drupal.org!";
    
   +    $actual_filtered_text = check_markup($text, 'filtered_html', '', FALSE, array());
   +    $this->verbose("Actual:<pre>$actual_filtered_text</pre>Expected:<pre>$expected_filtered_text</pre>");
        $this->assertIdentical(
   -      check_markup($text, 'filtered_html', '', FALSE, array()),
   

   This is very likely for a separate issue, but the filter logic is just begging to be moved to PHPUnit tests.

2. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,267 @@
   +/**
   + * Tests form elements provided by Filter module.
   + */
   +class FilterFormTest extends WebTestBase {
   +
   

   Or maybe is it relevant now. Can we not test the filter selection logic at the class level? If not, it means the class is not properly written. We should be able to just poke it and check the arrays that come back, no?

Thank you for picking this up; I have wanted per-node-type or per-field-instance format selection since Drupal 4.7. :-) This gets us a step closer to that.

Dur. Crap, you're right, I was reading too fast; The filter system still needs to be OOPified, but that's out of scope for this issue.

1. +++ b/core/modules/filter/filter.module
   @@ -512,9 +516,20 @@ function filter_process_format($element) {
   -  if (!isset($element['#format'])) {
   -    $element['#format'] = filter_default_format($user);
   ...
   +  if (isset($element['#allowed_formats'])) {
   ...
   +  if (!isset($element['#format']) && !empty($formats)) {
   

   In HEAD, there was no way to get past these lines without an $element['#format'] being set.

   Now with this patch, I could have $element['#allowed_formats'] = array();, and then $element['#format'] would be empty.

2. +++ b/core/modules/filter/filter.module
   @@ -512,9 +516,20 @@ function filter_process_format($element) {
   +    $formats = array_intersect_key($formats, array_flip($element['#allowed_formats']));
   

   One solution would be to add += array(filter_default_format($user)) here. At least I think that would work.

3. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterAPITest.php
   @@ -23,7 +23,7 @@ class FilterAPITest extends EntityUnitTestBase {
   -      'name' => 'API',
   +      'name' => 'Filter API',
   

   Um, okay.

4. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,270 @@
   +    /** @var \Drupal\filter\Entity\FilterFormat $filter_test_format */
   ...
   +    /** @var \Drupal\filter\Entity\FilterFormat $filtered_html_format */
   ...
   +    /** @var \Drupal\filter\Entity\FilterFormat $full_html_format */
   
   +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterSecurityTest.php
   @@ -44,19 +44,8 @@ function setUp() {
   +    /** @var \Drupal\filter\Entity\FilterFormat $filtered_html_format */
   

   These are entirely unnecessary here, but if you're going to bother with @var, at least use the interface not the class.

5. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterSecurityTest.php
   @@ -101,8 +90,8 @@ function testDisableFilterModule() {
      function testSkipSecurityFilters() {
   -    $text = "Text with some disallowed tags: <script />, <em><object>unicorn</object></em>, <i><table></i>.";
   -    $expected_filtered_text = "Text with some disallowed tags: , <em>unicorn</em>, .";
   +    $text = "Text with some disallowed tags: <script />, <p><object>unicorn</object></p>, <i><table></i>.";
   +    $expected_filtered_text = "Text with some disallowed tags: , <p>unicorn</p>, .";
   

   Why bother changing this? It makes me uncomfortable to mess with a test called "testSkipSecurityFilters" unless there is a good reason.

Marked #1693286: Allow administrator to specify available text format on per-field basis. as a duplicate.

47: 1423244-38-allowed-formats_0.patch queued for re-testing.

Nice work! :)

Manual testing revealed no regressions compared to D8 HEAD. However, tests do NOT pass locally, so requeued for testing.

I have found a lot of small things. The typos I fixed in this reroll, but still quite a few for you to address

1. +++ b/core/modules/filter/filter.module
   @@ -499,6 +502,7 @@ function filter_process_format($element) {
   +  $element['value'] += array('#default_value' => '');
   

   Why is this necessary?

2. +++ b/core/modules/filter/filter.module
   @@ -512,9 +516,20 @@ function filter_process_format($element) {
   +    // weight. This is equivalent to calling filter_default_format().
   +    $element['#format'] = reset($formats)->format;
   

   It is indeed equivalent. But AFAICT, as of this patch, we don't have any uses of filter_default_format() left. Why not remove it? :)

3. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,270 @@
   +class FilterFormTest extends WebTestBase {
   

   Any reason why this can't be a DUTB test?

4. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,270 @@
   +      'name' => 'Filter form element',
   

   "Text format form element"?

5. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,270 @@
   +    // 'filtered_html' as the default value in this case.
   

   'filter_test'?

6. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,270 @@
   +    $this->assertRequiredSelect('edit-all-formats-default-missing-format--2', $formats);
   

   I found it *very* confusing that for the previous cases, you would call assertOptions(), but in this case, you'd call assertRequiredSelect(), but not assert the options. After looking at the implementation details, it turns out that does happen, but the name is just a bit misleading. I think renaming it to assertRequiredSelectAndOptions() would clear that up completely :)

7. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,270 @@
   +    // default. This happens to be 'filter_test'.
   

   'filtered_html'?

8. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,270 @@
   +    // 'full_html' as the default value in this case.
   

   'filter_test'?

9. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
   @@ -0,0 +1,270 @@
   +    // The user only has access to the 'filter_test' format, so when no default
   +    // is given that is preselected and the text format select is hidden.
   +    $this->assertNoSelect('edit-restricted-formats-no-default-format--2');
   

   This should then still assert that the input[type=hidden] for the text format has a value of "filter_test".

10. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
    @@ -0,0 +1,270 @@
    +    $this->assertNoSelect('edit-single-format-default-format--2');
    

    Here too, there should be a verification of the input[type=hidden] value!

11. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterFormTest.php
    @@ -0,0 +1,270 @@
    +    // If the select has a missing or disallowed format make the administrator
    +    // explicitly choose the format.
    

    Wrong comment.

12. +++ b/core/modules/filter/tests/filter_test/lib/Drupal/filter_test/Form/FilterTestFormatForm.php
    @@ -0,0 +1,117 @@
    +      '#format' => 'basic_html',
    

    basic_html is undefined; filter_test only creates the filtered_html and full_html text formats, so you'll want to change this to filtered_html.

Plus, I found one part of filter_process_format() that I think needs some extra attention:

  // If multiple text formats are available, remove the fallback. The
  // "always_show_fallback_choice" is a hidden variable that has no UI. It
  // defaults to false.
  if (!\Drupal::config('filter.settings')->get('always_show_fallback_choice')) {
    $fallback_format = filter_fallback_format();
    if ($element['#format'] !== $fallback_format && count($formats) > 1) {
      unset($formats[$fallback_format]);
    }
  }

Particularly: count($formats) > 1. Thanks to #allow_formats, $formats can easily contain only one text format, which would make this kick in at undesired moments. I suggest changing the outer if-condition to:

  // If multiple text formats are available, remove the fallback. The
  // "always_show_fallback_choice" is a hidden variable that has no UI. It
  // defaults to false. However, whenever #allowed_formats is set, don't
  // remove the fallback, because the developer has meticulously
  // configured the list of allowed text formats.
  if (!\Drupal::config('filter.settings')->get('always_show_fallback_choice') && !isset($element['#allowed_formats'])) {

Queuing new patch for testing too.

Same failures, as expected :)

This is required to #2144413: Config translation does not support text elements with a format which is required to provide complete and definite schema support for views, so this not a feature, its a blocker task.

Almost there :)

9 & 10: on a fresh D8 install, grant the anonymous user permission to post comments, then go to the comment form as an anonymous users and inspect the comment form HTML. You'll find a input[type=hidden] with the text format.

1. +++ b/core/modules/filter/filter.module
   @@ -484,6 +484,7 @@ function filter_process_format($element) {
   +  // Make sure the #default_value key is set, so we can use it below.
   

   I can't find this usage below?

2. +++ b/core/modules/filter/filter.module
   @@ -513,10 +514,15 @@ function filter_process_format($element) {
   +  // 3. The fallback format is not the default format.
   

   I don't see this condition expressed in the if-test.

   Is this wrong, or is it implied but am I overlooking it?

0. I can't reproduce this myself. I must have confused this with something else.

1. Ok.

2. You're right. Thanks for clarifying — that should've been documented before, but it wasn't — my fault.

I don't have any other complaints. Especially because 90% of this patch is test coverage that we should've already had, and after carefully reviewing all added tests, I could not find any flaws. Even if this were to introduce a regression, the tests will make it trivial to fix.

Great work — thanks! :)

This looks great. Any reason not to backport it? I consider this a security improvement since people are going to try to modify the list of options anyway, and without this it's very difficult to do correctly (see #1295248: Allow per-field-instance configuration of allowed formats).

+ *   - #allowed_formats: (optional) An array of text format IDs that are
+ *     available for this element. If omitted, all text formats that the current
+ *     user has access to will be allowed.

It would be good to add something to the documentation to make clear that including a text format in #allowed_formats has no effect unless the user also has access to it (i.e. that it can only be used to restrict the default list, not add to it).

The tests look really good overall. They partially duplicate some of the existing tests in FilterFormatAccessTest.php (so ideally they'd be combined), but the method of testing for these new ones looks a bit cleaner, and a bit of duplicate testing doesn't hurt anyone :)

Meant to add the backport tag (optimistically assuming we can backport it).

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

+++ b/core/modules/filter/src/Tests/FilterFormTest.php
@@ -0,0 +1,271 @@
+  protected function doFilterFormTestAsNonAdmin() {
+    $this->drupalLogin($this->webUser);
+    $this->drupalGet('filter-test/text-format');
+
+    // Test a text format element with all formats. Only formats the user has
+    // access to are shown.
+    $formats = array('filtered_html', 'filter_test');
+    // If no default is given, the format with the lowest weight becomes the
+    // default. This happens to be 'filtered_html'.
+    $this->assertOptions('edit-all-formats-no-default-format--2', $formats, 'filtered_html');
+    // \Drupal\filter_test\Form\FilterTestFormatForm::buildForm() uses
+    // 'filter_test' as the default value in this case.
+    $this->assertOptions('edit-all-formats-default-format--2', $formats, 'filter_test');
+    // If a missing format is given as default, administers must select a valid
+    // replacement format.
+    $this->assertDisabledTextarea('edit-all-formats-default-missing-value');
+
+    // Test a text format element with a predefined list of formats.
+    // The user only has access to the 'filter_test' format, so when no default
+    // is given that is preselected and the text format select is hidden.
+    $this->assertNoSelect('edit-restricted-formats-no-default-format--2');
+    // When the format that the user does not have access to is preselected, the
+    // textarea should be disabled.
+    $this->assertDisabledTextarea('edit-restricted-formats-default-value');
+    $this->assertDisabledTextarea('edit-restricted-formats-default-missing-value');
+    $this->assertDisabledTextarea('edit-restricted-formats-default-disallowed-value');
+
+    // Test a text format element with a fixed format.
+    $formats = array('filtered_html');
+    // When there is only a single option there is no point in choosing.
+    $this->assertNoSelect('edit-single-format-no-default-format--2');
+    $this->assertNoSelect('edit-single-format-default-format--2');
+    // If the select has a missing or disallowed format make sure the textarea
+    // is disabled.
+    $this->assertDisabledTextarea('edit-single-format-default-missing-value');
+    $this->assertDisabledTextarea('edit-single-format-default-disallowed-value');
+  }
+

Should we not also be asserting on enabled text areas? Also the final $formats = array('filtered_html'); creates a variable that is never used - are we testing what we think we're testing?

All the changes look good to me, and address all of @alexpott's points.

Needs a reroll

git ac https://drupal.org/files/issues/1423244-65-allowed-formats.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 28944  100 28944    0     0  66635      0 --:--:-- --:--:-- --:--:-- 79298
error: patch failed: core/modules/filter/src/Tests/FilterAPITest.php:105
error: core/modules/filter/src/Tests/FilterAPITest.php: patch does not apply
error: patch failed: core/modules/filter/src/Tests/FilterSecurityTest.php:101
error: core/modules/filter/src/Tests/FilterSecurityTest.php: patch does not apply

Rerolled.

+++ b/core/modules/filter/src/Tests/FilterFormTest.php
@@ -0,0 +1,302 @@
+    return $select;

Should the return value be documented or is it obvious enough?

Thanks for the reroll! Same number of insertions and deletions + looks the same + green, so back to RTBC per #66.

Committed 7922db6 and pushed to 8.x. Thanks!

Let's handle the assert return value in a new followup issue.

+++ b/core/modules/filter/src/Tests/FilterFormTest.php
@@ -0,0 +1,302 @@
+  /**
+   * Makes sure that no select element with the given ID exists on the page.
+   *
+   * @param string $id
+   *   The HTML ID of the select element.
+   */
+  protected function assertNoSelect($id) {
...
+  /**
+   * Asserts that a select element has the correct options.
+   *
+   * @param string $id
+   *   The HTML ID of the select element.
+   * @param array $expected_options
+   *   An array of option values.
+   * @param string $selected
+   *   The value of the selected option.
+   */
+  protected function assertOptions($id, array $expected_options, $selected) {
...
+  /**
+   * Asserts that there is a select element with the given ID that is required.
+   *
+   * @param string $id
+   *   The HTML ID of the select element.
+   * @param array $options
+   *   An array of option values that are contained in the select element
+   *   besides the "- Select -" option.
+   */
+  protected function assertRequiredSelectAndOptions($id, array $options) {
...
+  /**
+   * Asserts that a textarea with a given ID exists and is not disabled.
+   *
+   * @param string $id
+   *   The HTML ID of the textarea.
+   */
+  protected function assertEnabledTextarea($id) {
...
+  /**
+   * Asserts that a textarea with a given ID has been disabled from editing.
+   *
+   * @param string $id
+   *   The HTML ID of the textarea.
+   */
+  protected function assertDisabledTextarea($id) {

All assertSomething() functions should return a bool indicating pass or fail.

   * @return
   *   TRUE if the assertion succeeded, FALSE otherwise.

I created that followup: #2284383: All assertSomething() functions should return a bool indicating pass or fail.