[regression] Disabled format still filters and displays the value

Nice find. And the test and fix is looking good.

+++ b/core/modules/filter/src/Element/ProcessedText.php
@@ -80,7 +80,7 @@ public static function preRenderText($element) {
       static::logger('filter')->alert('Missing text format: %format.', array('%format' => $format_id));

If it disabled it is not "missing".

+++ b/core/modules/filter/src/Element/ProcessedText.php
@@ -78,10 +78,13 @@ public static function preRenderText($element) {
+      $message = !$format ? 'Missing text format: %format.' : 'Disabled text format: %format.';

If we're going to make this distinction, we should test it.

+++ b/core/modules/filter/src/Element/ProcessedText.php
@@ -78,10 +78,13 @@ public static function preRenderText($element) {
+    if (!$format || !$format->get('status')) {

We have $format->status() available to us.

However, this doesn't take the fallback filter into account. Sure, in theory it can't be disabled, but still.

Thirdly, we have $format->access('use'), which for some reason does not check status.

We should get this right, this should never have been a bug.

---

Maybe something like this? Not sure how/if to fix FilterFormatAccessControlHandler.

+++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -150,6 +150,13 @@ public function filters($instance_id = NULL) {
+    return parent::status() || $this->isFallbackFormat();

We should explain this particular line with a comment.

+++ b/core/modules/filter/src/Entity/FilterFormat.php
@@ -150,6 +150,13 @@ public function filters($instance_id = NULL) {
   /**
    * {@inheritdoc}
    */
+  public function status() {
+    return parent::status() || $this->isFallbackFormat();
+  }

Would it not be better to throw an exception in the disable() method if someone tries to disable the fallback format?

@claudiu.cristea yep and then there is no need to override status in FilterFormat

+++ b/core/modules/filter/src/Tests/FilterCrudTest.php
@@ -74,6 +75,23 @@ function testTextFormatCrud() {
+      if ($e->getMessage() == "The fallback text format 'plain_text' cannot be disabled.") {
+        $this->pass($message);
+      }

This should just do an assertion against the message... doing an if means that a LogicException with a different message would produce a false positive.

+++ b/core/modules/filter/src/Element/ProcessedText.php
@@ -78,10 +78,13 @@ public static function preRenderText($element) {
+      $message = !$format ? 'Missing text format: %format.' : 'Disabled text format: %format.';

We still don't test the 'Missing text format' message. Shouldn't we?

Something like this maybe?

@claudiu.cristea, sorry patch in #29 is missing the FilterStatic.php file, #27 should probably be re-uploaded then

The actual changes look good, but the tests need a bit of work:

1. +++ b/core/modules/filter/src/Element/ProcessedText.php
   @@ -78,10 +78,13 @@ public static function preRenderText($element) {
   +    // If the requested text format doesn't exist or it's disabled, the text
   

   Nit: s/it's/is/

2. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -386,4 +390,60 @@ function testFilterTipHtmlEscape() {
   +   * Tests whether a field having a disabled format is rendered.
   

   Nit: s/having/using/

3. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -386,4 +390,60 @@ function testFilterTipHtmlEscape() {
   +    // We test the interface as anonymous user.
   

   Why?

   And if we indeed want to keep this: s/as/as the/

4. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -386,4 +390,60 @@ function testFilterTipHtmlEscape() {
   +    // Create a new format and add a filter that replaces the text with a static
   +    // text every time.
   

   The wording here feels a bit off. What about: Create a text format with a filter that returns a static string.

5. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -386,4 +390,60 @@ function testFilterTipHtmlEscape() {
   +    /** @var \Drupal\filter\FilterFormatInterface $format */
   

   Unnecessary, can be deleted.

6. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -386,4 +390,60 @@ function testFilterTipHtmlEscape() {
   +    $format->save();
   +    $format->setFilterConfig('filter_static_text', ['status' => TRUE]);
   +    $format->save();
   

   This first $format->save() can be removed.

7. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -386,4 +390,60 @@ function testFilterTipHtmlEscape() {
   +    $body_value = 'body text &<';
   ...
   +    $this->assertNoRaw($body_value);
   +    $this->assertNoEscaped($body_value);
   

   Why this very special string if we are merely asserting the absence? Seems like it could simply be $body_value = $this->randomName().

8. +++ b/core/modules/filter/tests/filter_test_plugin/src/Plugin/Filter/FilterStatic.php
   @@ -0,0 +1,33 @@
   +class FilterStatic extends FilterBase {
   

   s/FilterStatic/FilterTestStatic/

   (All other test-only filters are named FilterTestSOMETHING.)

   Also, why isn't FilterTestReplace already sufficient for the needs of the tests added in this patch?

Looking into it.

Why?

And if we indeed want to keep this: s/as/as the/

The test passed, so I just removed it.

Why this very special string if we are merely asserting the absence? Seems like it could simply be $body_value = $this->randomName().

Yeah we add both & and < in the random string code.

Also, why isn't FilterTestReplace already sufficient for the needs of the tests added in this patch?

Well, the other one seems to do more, and we want to stay explicit here ...

1. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -180,9 +180,10 @@ public function toArray() {
   -      throw new \LogicException(sprintf("The fallback text format '%s' cannot be disabled.", $this->id()));
   +      throw new \LogicException("The fallback text format '{$this->id()}' cannot be disabled.");
   

   Good call!

2. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -418,20 +412,20 @@ public function testDisabledFormat() {
   -    $this->drupalGet('node/' . $node->id());
   +    $this->drupalGet($node->urlInfo());
   

   Again good call :)

3. +++ b/core/modules/filter/src/Entity/FilterFormat.php
   @@ -180,6 +180,12 @@ public function toArray() {
   +    // The fallback text format cannot be disabled, as we always have to be able
   +    // to run it, in case a different format is disabled.
   

   This could be improved by a native speaker on commit.

4. +++ b/core/modules/filter/tests/filter_test_plugin/src/Plugin/Filter/FilterTestStatic.php
   @@ -0,0 +1,33 @@
   + * Provides a filter that returns the same static text, regardless of what input
   + * text he receives.
   

   I think the "regardless of what input text he receives" can simply be omitted. Can be fixed on commit.

I quickly fixed #39.3 and 4 myself. In fact, the entire comment mentioned in 3 can be deleted, "fallback format" already says plenty.

Keeping at RTBC because these are just minor comment changes.

Committed bb39343 and pushed to 8.0.x. Thanks!