Problem/Motivation

Now that #1559310: 404 pages should be language aware is in, an option to suppress 404 messages being logged would be useful to sites that get a lot of 404 requests.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Did some quick research, looks like the best way to do this is to add a decorator for the logger factory and return a null logger for the page not found channel.

jeetmail72’s picture

Hi @Berdir,

Can you please explain the requirement that you are looking from this module.

tduong’s picture

Assigned: Unassigned » tduong
tduong’s picture

- added the suppress_404 setting form in admin/config/search/redirect/settings
- implemented a decorator service with logger factory to avoid logging 404 events if the setting is enabled, otherwise let the default logger do its work
- added test coverage

Status: Needs review » Needs work

The last submitted patch, 5: add_an_option_to-2836458-5.patch, failed testing.

tduong’s picture

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/modules/redirect_404/config/schema/redirect_404.schema.yml
    @@ -9,3 +9,6 @@ redirect_404.settings:
    +      label: '404 logged messages to be suppressed.'
    

    this label can be improved: Whether to log page not found messages to the standard log or not

  2. +++ b/modules/redirect_404/redirect_404.module
    @@ -49,6 +51,13 @@ function redirect_404_form_redirect_settings_form_alter(&$form, FormStateInterfa
    +    '#title' => t('Suppress 404'),
    

    Suppress "page not found" log messages

  3. +++ b/modules/redirect_404/redirect_404.module
    @@ -49,6 +51,13 @@ function redirect_404_form_redirect_settings_form_alter(&$form, FormStateInterfa
    +    '#description' => t('404 logged messages to be suppressed, which are not needed for redirect_404 to run. Enable this setting if your site gets a lot of 404 requests.'),
    

    Prevents page not found log messages, can be safely enabled when this module is used which stores them separately and nothing else relies on those messages.

  4. +++ b/modules/redirect_404/src/Render/Redirect404SuppressLogger.php
    @@ -0,0 +1,63 @@
    +
    +/**
    + * Suppresses logged 404 messages that are not needed for redirect_404 to run.
    + */
    

    that it can is not really an important information here. I'd write "Allows page not found log messages to be suppressed by returning a NullLogger".

  5. +++ b/modules/redirect_404/src/Render/Redirect404SuppressLogger.php
    @@ -0,0 +1,63 @@
    +class Redirect404SuppressLogger extends LoggerChannelFactory {
    

    Redirect404LogSuppressor?

  6. +++ b/modules/redirect_404/src/Render/Redirect404SuppressLogger.php
    @@ -0,0 +1,63 @@
    +    $this->logger = $logger;
    

    use loggerChannelFactory and $logger_channel_factory.

  7. +++ b/modules/redirect_404/src/Render/Redirect404SuppressLogger.php
    @@ -0,0 +1,63 @@
    +    $this->config = $config->get('redirect_404.settings');
    

    error logging can happen very early, we don't know for sure that the config is readable and so on. It might also not be needed.

    just assign this as $configFactory only read the config in case we have a page not found $channel request.

  8. +++ b/modules/redirect_404/src/Render/Redirect404SuppressLogger.php
    @@ -0,0 +1,63 @@
    +    if ($this->config->get('suppress_404') && $channel == 'page not found') {
    +      // Do not log if the suppress_404 is enabled and a 404 error is detected.
    +      return new NullLogger();
    +    }
    

    the the order then and read from $this->configFactory->get()...

  9. +++ b/modules/redirect_404/src/Render/Redirect404SuppressLogger.php
    @@ -0,0 +1,63 @@
    +  public function addLogger(LoggerInterface $logger, $priority = 0) {
    +    parent::addLogger($logger, $priority);
    +  }
    

    this is weird, because there will then be two different instances, ourself with the parent and the inner.

    Do not extend, only implement the interface. And call $this->loggerChannelFactory instead.

  10. +++ b/modules/redirect_404/src/Tests/Redirect404SuppressLoggerTest.php
    @@ -0,0 +1,86 @@
    +    // Create users with specific permissions.
    +    $this->adminUser = $this->drupalCreateUser([
    +      'administer redirects',
    +      'administer redirect settings',
    +      'access site reports',
    +      'access content',
    +      'bypass node access',
    +      'administer site configuration',
    +      'access administration pages',
    +      'administer users',
    +    ]);
    +    $this->webUser = $this->drupalCreateUser([]);
    

    we can probably skip a good amount of those permission as we don't need the dblog UI.

  11. +++ b/modules/redirect_404/src/Tests/Redirect404SuppressLoggerTest.php
    @@ -0,0 +1,86 @@
    +    $this->assertEqual(db_query("SELECT COUNT(*) FROM {watchdog} WHERE type LIKE 'page not found'")->fetchField(), 1);
    +    $this->assertEqual(db_query("SELECT COUNT(*) FROM {watchdog} WHERE type LIKE 'access denied'")->fetchField(), 1);
    

    no need to use LIKE

tduong’s picture

Addressed comment #8

Status: Needs review » Needs work

The last submitted patch, 9: add_an_option_to-2836458-9.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

  • Berdir committed 377a9d2 on 8.x-1.x authored by tduong
    Issue #2836458 by tduong: Add an option to suppress page not found...
Berdir’s picture

Status: Needs review » Fixed

That interdiff was pretty useless :)

Changes look good though, committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jenlampton’s picture

Hi all, thanks for working on this much needed feature. Is there a matching Drupal 7 issue? I've got a handful of D7 sites I'd like to use this on.

JasonLuttrell’s picture

I would like to +1 or second the comment by jenlampton. I have some legacy D7 sites that have this issue, it would be great if someone can back port a fix like this or let us know if one is already available. Thanks!