Do not use basic_html text format for 'No log messages available.' message

Thanks! Makes sense. With this change we could also get rid of the code of the method DbLogViewsTest::testDBLogAddAndClear()

Could you do that in your patch?

Here is the proposed change.

Works as expected.
I do not believe it needs additional tests as the test working around the issue has been removed.

This lead me to open #2917817: Views text area plugin does not declare a dependency on the text format it uses. That would have made this error properly visible and would properly prevent installation of a view with a missing text format. This issue is still valid, though, and the fix makes sense in my opinion.

A different solution would be to use the text_custom area plugin to output markup directly. I think it makes sense, though, to avoid #markup for user-entered text, though I think the patch is good as is.

One thing that strikes me as a bit tricky is translations. Sometimes a translation for something can involve html to get it right. With this change I can't use html entities like é. See:

I'm not sure what the solution here is.

Well, people who need that could simply add a proper text format, which I personally think is acceptable. The other way would be what I alluded to in #6 and simply use the custom_text area which doesn't use a format at all.

Yep looking at \Drupal\views\Plugin\views\area\TextCustom() that is exactly what we should use.

  /**
   * Render a text area with \Drupal\Component\Utility\Xss::filterAdmin().
   */
  public function renderTextarea($value) {
    if ($value) {
      return $this->sanitizeValue($this->tokenizeValue($value), 'xss_admin');
    }
  }

I think applying the xss_admin filter for admin text makes sense.

Ahh, I didn't realize we had the xss_admin there. I guess in that case I agree that that is the best solution as it seems to be a good tradeoff between flexibility, complexity and security.

Here is my attempt at the custom_text implementation of this bug. I believe the test method is still relevant to delete here.

Thanks!. I checked this and is working as expected. The only thing that doesn't convince me is the administrative title available in Views.

Could you change it to indicate that this is the "No recent logs message"?

We should also file an issue for area plugin's to add a dependency on the text format.

Plus there is a question about the upgrade path. I think if the current active configuration matches the old configuration we should update it.

Re #13.1: See #2917817: Views text area plugin does not declare a dependency on the text format it uses which I had already opened in #6.

I think if the current active configuration matches the old configuration we should update it.

@alexpott So basically, if Current Log message Text == 'No log messages available.' then update the config?

Hmm... happy to update my patch based on #12 and #13, but I think I need a little more help @alexpott. Do you know of any other issues that do something like this? Would this be an update hook in dblog? Would I manually check the view config to see if the old setting exists, and update it if it does?

@dpagini Thanks! There is an example of this type of upgrade path and tests here: #2851293: dblog is using the wrong views field, filter and relationships definitions

I am seeing this on my dev site.

A suggestion: I think you need to define a new text format in config/optional in the dblog module, and use that text format in the view. That way you are not depending on the text format basic_html that is defined by the standard install profile, and may not exist on all sites.

But yes, in addition, the view should know that it is depending on both the filter module and the basic_html text format. Well... it looks like you can't install Views without the filter module, so I guess that can be left out (views.info.yml lists filter module as a dependency).

In my opinion, creating filter_html as part of the dblog module seems a lot for just a string, I think the solution proposed in #11 is the way yo go.

Either way. But if for some reason the module needs a text format for its view, it should provide that format in config/optional rather than assuming a format with a particular machine name exists.

Here is the upgrade path with tests.

Using drupal 8.4. database version this patch is smaller.

If this pass I think is ready for a final review.

Question: In DbLogViewsTest.php, is the parent testDBLogAndClear() method still being run if it is not overridden in the class? I am not sure exactly how the test methods to run discovery process works, so I'm not sure if we remove the override of the parent method, if it still gets recognized as a test method to run. Can we verify that?

Other than that, I think the code and test coverage look good.

Minor nitpick on the patch:

core/modules/dblog/tests/src/Functional/DbLogViewsTest.php

   /**
-   * {@inheritdoc}
+   * Test the empty text for the watchdog view is not using an input format.
    */

Docs line should start with "Tests" not "Test".

Thanks @jhodgdon

Question: In DbLogViewsTest.php, is the parent testDBLogAndClear() method still being run if it is not overridden in the class? I am not sure exactly how the test methods to run discovery process works, so I'm not sure if we remove the override of the parent method, if it still gets recognized as a test method to run. Can we verify that?

@jhodgdon yes, by default all non private methods are inherited by child classes. Actually that method was overridden just to create the input format, now that is not required we can use the tests defined by the parent class.

Docs line should start with "Tests" not "Test".

Fixed.

I know that all methods (even private actually) are inherited. My question is whether the test runner, which decides which methods to run based on some criteria (like, I think they have to start with "test" and have no arguments and be public?) also runs inherited methods. Again, I don't know what the criteria for deciding which methods to run, or what the discovery process is, so I would like to see verification that this method is actually being run after having its override removed.

@jhodgdon sorry, my bad. Any public method starting with test will be executed.

• https://www.drupal.org/docs/8/testing/simpletest-class-file-and-namespac... (I just updated the docs to make emphasis on public methods)
• https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html

Checking the test runner results you can check that DbLogViewsTest executed all the tests defined by DbLogTest + the one that this patch adds.

Drupal\Tests\dblog\Functional\DbLogTest                        7 passes
Drupal\Tests\dblog\Functional\DbLogViewsTest                   8 passes

Right, duh! You can actually see the individual methods that are run in the test results, if you click the "All" link to show all (not just failing) classes.

The test results do have one coding standards error, very minor:

/var/lib/drupalci/workspace/jenkins-drupal_patches-53916/source/core/modules/dblog/tests/src/Functional/DbLogViewsTest.php
✗ 1 more line 58 The closing brace for the class must have an empty line before it

So I think if that is fixed, this will be ready to go. Thanks!

Fixed coding standard.

OOps. I just noticed a typo: testEmtpyText() method -> empty not emtpy. :) Other than that this looks RTBC to me, so I just edited the patch to switch those two characters.

Also adding a note to the issue summary for reviewers/committers who might be confused about the test coverage.

Committed 749b7d7 and pushed to 8.6.x. Thanks!

is this patch included in version 8.5 ?

Thanks.

It looks like it was only committed to 8.6, not 8.5.

Hi,
this is causing the following error with Drupal 8.6.5 and PHP 7.1 when enabling any module via drush.

TypeError: Argument 2 passed to Drupal\locale\LocaleConfigManager::filterOverride() must be of the type array, object given, called in /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php on line 639 in Drupal\locale\LocaleConfigManager->filterOverride() (line 629 of /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php) #0 /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php(639): Drupal\locale\LocaleConfigManager->filterOverride(Array, Object(Drupal\Core\StringTranslation\TranslatableMarkup))
#1 /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php(639): Drupal\locale\LocaleConfigManager->filterOverride(Array, Array)
#2 /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php(639): Drupal\locale\LocaleConfigManager->filterOverride(Array, Array)
#3 /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php(639): Drupal\locale\LocaleConfigManager->filterOverride(Array, Array)
#4 /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php(639): Drupal\locale\LocaleConfigManager->filterOverride(Array, Array)
#5 /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php(639): Drupal\locale\LocaleConfigManager->filterOverride(Array, Array)
#6 /var/www/html/web/core/modules/locale/src/LocaleConfigManager.php(587): Drupal\locale\LocaleConfigManager->filterOverride(Array, Array)
#7 /var/www/html/web/core/modules/locale/locale.bulk.inc(609): Drupal\locale\LocaleConfigManager->updateConfigTranslations(Array, Array)
#8 /var/www/html/vendor/drush/drush/includes/batch.inc(235): locale_config_batch_refresh_name(Array, Array, Object(DrushBatchContext))
#9 /var/www/html/vendor/drush/drush/includes/batch.inc(183): _drush_batch_worker()
#10 /var/www/html/vendor/drush/drush/includes/batch.inc(95): _drush_batch_command('640')
#11 /var/www/html/vendor/drush/drush/src/Drupal/Commands/core/BatchCommands.php(19): drush_batch_command('640')
#12 [internal function]: Drush\Drupal\Commands\core\BatchCommands->process('640', Array)
#13 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(235): call_user_func_array(Array, Array)
#14 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(181): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolidation\AnnotatedCommand\CommandData))
#15 /var/www/html/vendor/consolidation/annotated-command/src/CommandProcessor.php(150): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#16 /var/www/html/vendor/consolidation/annotated-command/src/AnnotatedCommand.php(404): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
#17 /var/www/html/vendor/symfony/console/Command/Command.php(255): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Drush\Symfony\LessStrictArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#18 /var/www/html/vendor/symfony/console/Application.php(971): Symfony\Component\Console\Command\Command->run(Object(Drush\Symfony\LessStrictArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#19 /var/www/html/vendor/symfony/console/Application.php(248): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object(Drush\Symfony\LessStrictArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#20 /var/www/html/vendor/symfony/console/Application.php(148): Symfony\Component\Console\Application->doRun(Object(Drush\Symfony\LessStrictArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#21 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(112): Symfony\Component\Console\Application->run(Object(Drush\Symfony\LessStrictArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
#22 /var/www/html/vendor/drush/drush/src/Runtime/Runtime.php(41): Drush\Runtime\Runtime->doRun(Array)
#23 /var/www/html/vendor/drush/drush/drush.php(66): Drush\Runtime\Runtime->run(Array)
#24 /var/www/html/vendor/drush/drush/drush(4): require('/var/www/html/v...')
#25 {main}.

The object it's complaining about is

class Drupal\Core\StringTranslation\TranslatableMarkup#16758 (5) {
  protected $translatedMarkup =>
  NULL
  protected $options =>
  array(0) {
  }
  protected $stringTranslation =>
  NULL
  protected $string =>
  string(26) "No log messages available."
  protected $arguments =>
  array(0) {
  }
}

That's the "display.default.display_options.empty.area.content" field in core/modules/dblog/config/optional/views.view.watchdog.yml. Setting the content field as an empty strings clears the error, but obviously that isn't a sufficient solution.

Hello,
Sorry about commenting again this issue, I'll leave here the comment to have it spotted. I've just run into it and I have been taking a look at the commit https://git.drupalcode.org/project/drupal/commit/749b7d7.
It looks like it is missing something, while it changes id: area_text_custom it is not changing the area plugin type to "area_text_custom".

      empty:
        area:
          id: area_text_custom
          table: views
          field: area_text_custom
          relationship: none
          group_type: group
          admin_label: 'No log messages available.'
          empty: true
          tokenize: false
          content: 'No log messages available.'
          plugin_id: text_custom

Something like:

      empty:
        area_text_custom:
          id: area_text_custom
          table: views
          field: area_text_custom
          relationship: none
          group_type: group
          admin_label: 'No log messages available.'
          empty: true
          tokenize: false
          content: 'No log messages available.'
          plugin_id: text_custom

I am not really familiar with configuration at this level, but installing a multi site from a custom profile with language different than english, and enabling a default_content module will make locale to run locale_system_set_config_langcodes() which endeds up trying to save $config->set('langcode', $default_langcode)->save() on the views.view.watchdog raising an InvalidArgumentException because Placeholders must have a trailing [] if they are to be expanded with an array of values..

Changing (hardcoding for PoC) the area TO area_text_custom fixed the Exception.

Just to let you know since I've just run into it.

@TuWebO

Thank you! you're my rescuer... Couldn't find the problem during the drupal update to 8.8.1 - I had this problem in views.view.watchdog.yml Replaced to what you said and BAMM it works!

@TuWebO & @nortmas

Confirming the area TO area_text_custom fixed the issue!

thanks :)

This issue was fixed in 8.x actually, not sure why the version was changed? It does look like it may need a follow-up issue to fix problems...