404 pages should be language aware

Can confirm this, had the same situation today on a site that confused the customer and resulted in him adding redirects for existing pages.

Not sure if it can be fixed, however. Core would have to log the original path before the language prefix was removed, redirect.module only displays whatever information is in the page not found log.

I guess if data from core is missing, redirect should capture it on its own...

There's no specific 404 / page_not_found hook.

A 404 is created here finally via deliver_page:
http://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_n...

The problem with fast404 is, we can NOT react on them so they're missing in the reports.

So let's create a new error table in redirect as submodule.

Then record error data for 404s via something like hook_page_build() if enabled (by default) including language.
Additionally, let's import apaches error.log in a cron. Thus we can drop complexity on 404s (no inserts) AND we can even import fast404's.
We need to check for logrotate cases (if growing, incremental. If smaller, restart. Or always check first line for change.)

Finally we can offer a batch import of error.log's if they where missing.

I'd be happy to hear from the maintainer for his opinion and if we should go ahead.
This all is also possible in a separate contrib.

Hrm, I am wondering if redirect should implements it's own hook_watchdog() to record only 404 responses in it's own table rather than relying on dblog. It would make sense.

Additionally, let's import apaches error.log in a cron. Thus we can drop complexity on 404s (no inserts) AND we can even import fast404's.
We need to check for logrotate cases (if growing, incremental. If smaller, restart. Or always check first line for change.)

Finally we can offer a batch import of error.log's if they where missing.

This is out of scope for the module since it's implementation specific and could live separately in another module that feeds in the redirect 404 log table. And wouldn't raw server logs also have the same problem of missing the language context?

Raw server logs perfectly contain the language prefix (if it's a path prefix).
Drupal cuts this off in bootstrap with custom code.

hook_watchdog is a good idea. didn't find that...

We will end up with two db writes per 404. And if we not only append lines but e.g. try to select url & update counter, we will even end in a read-write-cycle, making most db's (especially with indexes) slow.

Then a variable to disable the 404 writes would be enough (so we can rely on the table and UI)

There is no counter. It's only one write per 404 as far as redirect would be concerned. Not any different from having dblog do a write per 404 or 403. And yeah, making it controlled by a variable (enabled by default) so that other methods could populate the table would give the most flexibility.

Here's a first step.
Introducing the schema table redirect_error, capturing all drupal_not_found as records.
Not really tested yet. Let's check testbots statuses.

Missing is

• Data migration from watchdog
• Test coverage
• Capture language data correct
• Pass language to redirect on creation

Also check the code @todo.

Hmm, testing postponed... It seems redirect has an exception in the tests now "Infinite redirect loop prevented."

+++ b/redirect.installundefined
@@ -356,3 +357,68 @@ function _redirect_migrate_path_redirect_variables() {
+  db_create_table('redirect_error', _redirect_schema_redirect_error());

The table definition needs to be copied, a helper function doesn't work. The schema could change and then you need to change hook_schema() but not the initial update function.

+++ b/redirect.installundefined
@@ -356,3 +357,68 @@ function _redirect_migrate_path_redirect_variables() {
+  // @todo migrate data from watchdog?
+  $return = '';
+
+  return $return;

No need to return anything if there is no relevant information.

Not sure if we need a data migration? This will get built up again and watchdog is only temporary data anyway.

+++ b/redirect.installundefined
@@ -356,3 +357,68 @@ function _redirect_migrate_path_redirect_variables() {
+      'uid' => array(
+        'type' => 'int',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+        'default' => 0,
+        'description' => 'The {users}.uid of the user who created the redirect.',

description seems wrong?

+++ b/redirect.installundefined
@@ -356,3 +357,68 @@ function _redirect_migrate_path_redirect_variables() {
+      'status_code' => array(
+        'type' => 'int',
+        'size' => 'small',
+        'not null' => TRUE,
+        'description' => 'The HTTP status code to use for the redirect.',

What's this for?

+++ b/redirect.installundefined
@@ -356,3 +357,68 @@ function _redirect_migrate_path_redirect_variables() {
+      'timestamp' => array(
+        'type' => 'int',
+        'unsigned' => TRUE,
+        'not null' => TRUE,
+        'default' => 0,
+        'description' => 'The timestamp of when the redirect was last accessed.'

Description also wrong I think.

+++ b/redirect.moduleundefined
@@ -1565,3 +1565,24 @@ function redirect_redirect_operations() {
+  if ($log_entry['type']=='page not found') {

Missing spaces around ==

+++ b/redirect.moduleundefined
@@ -1565,3 +1565,24 @@ function redirect_redirect_operations() {
+      // @todo fix for cases where language prefix is missing.
+      'language' => $language->language,

Not sure I understand the @todo, there is always a $language object and ->language is also always set. This should be fine I think.

Schema: Fully agree with the needed schema definition duplication. I was just looking a bit around and did a copy paste from ???

Most minor thingies. Most fixed already.
Migration dropped.

I added a status column to cover e.g. other 40x errors if needed...
This corresponds to the status code, a redirect sends (302, ...) and how redirect works.
In addition to 404 i thought about covering the case 403 (access denied). But note that a redirect doesn't really make sense there...
So yes, removed

'language' => $language->language,
The problem here is that $language has always a value.
If for example a callback comes WITHOUT a language prefix, we want to identify this case and add a language neutral redirect.
Since drupal has a default language, the default context is identified and only a redirect for that default language is added.
At least in case of switching the default language, all the added redirects don't work again.

I guess the right direction is to be found here:
http://api.drupal.org/api/drupal/includes%21language.inc/group/language_...
module_load_include('language', 'inc', 'language.negotiation');
$langcode = locale_language_from_url($languages);
http://api.drupal.org/api/drupal/includes%21locale.inc/function/locale_l...

Additionally added detection of multilingual case.

Missing is still

• Test coverage
• Pass language to redirect on creation

Both patches give me this raw code displayed on every page:

return $operations; } /** * Implements hook_watchdog. * * Write a redirect_error per 404. */ function redirect_watchdog(array $log_entry) { if ($log_entry['type'] == 'page not found') { if (!variable_get('redirect_error_log', TRUE)) { return; } global $user; global $language; $langcode = $language->language; if (module_exists('locale')) { require_once DRUPAL_ROOT . '/includes/language.inc'; module_load_include('locale.inc', 'locale'); // Fetch language from URL. $languages = language_list(); // We need a workaround for language negotiation as the language is already cutoff from $_GET['q']. $saveq = $_GET['q']; $_GET['q'] = request_path(); $url_langcode = locale_language_from_url($languages); $langcode = $url_langcode===FALSE?'':$url_langcode; $_GET['q'] = $saveq; } // Write record. $record = array( 'source' => $log_entry['message'], 'uid' => $user->uid, // @todo fix for cases where language prefix is missing. 'language' => $langcode, 'status_code' => '404', 'timestamp' => $log_entry['timestamp'], ); drupal_write_record('redirect_error', $record); } 

And these kind of errors:

Warning: Cannot modify header information - headers already sent by (output started at /home/altex/public_html/sites/all/modules/redirect/redirect.module:40) in drupal_send_headers() (line 1228 of /home/altex/public_html/includes/bootstrap.inc).
Warning: Cannot modify header information - headers already sent by (output started at /home/altex/public_html/sites/all/modules/redirect/redirect.module:40) in drupal_goto() (regel 698 van /home/altex/public_html/includes/common.inc).

I'm using Redirect 7.x-1.0-rc1+8-dev (2014-Jul-23).

Added this to our 8.x port board.

Yesterday redirect has been pushed to d.o, so changed version to 8.x-1.x-dev and the PR #98 will be moved here.

Uploaded current patch: first point of the PR is done + starting code for the RedirectNotFoundStorage service.

Fixed/completed RedirectNotFoundStorage service.
There are still a \Drupal::database() call in RedirectFix404Form (line 80) and redirect.install (line 230). Should we use a service also for them ?

If we're using our own table (which is good), I think we should write Views integration instead and use that to display this page, instead of hard-coded page output.

Debatable also if this should be moved to its own separate sub-module (a redirect_404 sub-module, and also splitting out the 'expire + counting + last access' into a redirect_stats sub-module).

1. +++ b/src/Entity/Redirect.php
   @@ -31,7 +31,8 @@ use Drupal\link\LinkItemInterface;
     *     "views_data" = "Drupal\views\EntityViewsData",
   - *     "storage_schema" = "\Drupal\redirect\RedirectStorageSchema"
   + *     "storage_schema" = "\Drupal\redirect\RedirectStorageSchema",
   + *     "storage_not_found" = "\Drupal\redirect\RedirectNotFoundStorage",
     *   },
   

   this is not an entity handler, just the service is enough. so you can remove this again.

2. +++ b/src/EventSubscriber/Redirect404Subscriber.php
   @@ -0,0 +1,47 @@
   +      if (!\Drupal::config('redirect.settings')->get('log_404')) {
   +        return;
   

   and config factory should be injected too.

3. +++ b/src/EventSubscriber/Redirect404Subscriber.php
   @@ -0,0 +1,47 @@
   +
   +      /** @var \Drupal\redirect\RedirectNotFoundStorage $redirect_storage */
   +      $redirect_storage = \Drupal::service('redirect.not_found_storage');
   +      $redirect_storage->mergeRedirect404Records();
   

   the storage shouldn't have to know about where the path is coming from. Make a method with specific arguments, something like logRequest($path, $langcode)

   You should also inject the storage into this service.

4. +++ b/src/Form/RedirectFix404Form.php
   @@ -55,72 +47,69 @@ class RedirectFix404Form extends FormBase {
   +    $query = $redirect_storage->getRedirect404Table($header);
   

   this should not return a query but just plain result objects.

   The idea of the service is that it must be able to implement it with a different backend that's not about sql at all.

   So you need to pass the $search string to it as well.

   $header is a bit tricky, the paging as well.

   About the method name, I'd do something like listRequests() or so.

5. +++ b/src/Form/RedirectFix404Form.php
   @@ -55,72 +47,69 @@ class RedirectFix404Form extends FormBase {
   +      $wildcard = '%' . trim(preg_replace('!\*+!', '%', \Drupal::database()->escapeLike($search)), '%') . '%';
   

   then this problem will resolve itself as it will be inside the service as well.

Crossposted with Dave, I'm OK with making this a separate redirect_404 module, instead of a setting.

Note that I suggested to make storage a service so that a faster backend like redis could be used. That conflicts with using views for it. Not sure.

Also note that the count/expire stuff has been removed already from redirect 8.x-1.x as it simply doesn't work with how we do caching now. So it would be re-implementing in a new module, not splitting :) And if it should work with page cache, it would have to be a middleware that runs before page cache.

Fixed points in the comment above, just "$header is a bit tricky, the paging as well" left.
Fixed RedirectUILanguageTest test + some refactoring.

Moving stuff into the sub-module will be done later as it would be hard to review.

The test fails because it is still using the old redirect_error table fields. Updates in RedirectUILanguageTest and RedirectFix404Form.

Fixed a line in RedirectUITest, but there is still some bugs that I don't know how to fix. It seems like when you want to add a redirect, it does not have a placeholder/pre-set a value in redirect_source[0][path] textfield, neither is language[0][value] (default to - Not specified - (und)). So these are 2 failing assertions, plus since Path field is required and has no pre-filled path value, after saving it "redirects back" to the same page, thus another assert fail.
Both of them appear in RedirectUITest and RedirectUILanguageTest.

Furthermore in the language test, from count(db_select('redirect_404')->fields('redirect_404')->condition('path', 'testing')->execute()->fetchAll()) it returns 0, even though it should be 1 (I've tried also with condition('count', 'testing')).

Then I've tested it manually and I'm not sure if it was meant to be so, but in the "Fix 404 pages" it has dropped the langcode from the path that was part of it if you see how it looks like without the patch applied (didn't check the past commits yet).

I'm not sure how to fix these bugs left. Will wait for some feedbacks and reviews/approval to move the redirect_404 stuff into a submodule.

Moved (almost) every 404 related files/codes into redirect_404 submodule, fixed where was needed.

Didn't run the tests before uploading the patch, but noticed that Fix404RedirectUILanguageTest takes more than 6 mins... what is wrong here ?

Dropped invalid taxonomy permission in both tests.

For the fix 404 redirect form *not actually* pre-filling its fields, I think that it may be a bug form RedirectFix404Form::buildForm() which leads to RedirectForm::prepareEntity() with no value for redirect_source[0][path] and language[0][value] (set as und per default), but not sure what is exactly the bug...

1. +++ b/modules/redirect_404/config/install/redirect_404.settings.yml
   @@ -0,0 +1 @@
   +log_404: true
   
   +++ b/modules/redirect_404/config/schema/redirect_404.schema.yml
   @@ -0,0 +1,9 @@
   +redirect_404.settings:
   +  type: config_entity
   +  label: 'Redirect 404 settings'
   +  mapping:
   +    log_404:
   +      type: boolean
   +      label: 'Log 404 errors'
   

   I think we no longer need the setting then. If you don't want this feature, don't enable the module.

2. +++ b/modules/redirect_404/redirect_404.info.yml
   @@ -0,0 +1,10 @@
   +dependencies:
   + - link
   + - views
   + - redirect
   

   we only need to specify redirect as dependency I think.

3. +++ b/modules/redirect_404/redirect_404.install
   @@ -0,0 +1,96 @@
   + * Add redirect_404 table.
   + */
   +function redirect_404_update_8104() {
   +  $schema = [
   

   We don't need an update anymore. All sites will have to enable the new module, which will install the table.

4. +++ b/modules/redirect_404/redirect_404.services.yml
   @@ -0,0 +1,10 @@
   +  redirect.not_found_storage:
   +    class: Drupal\redirect_404\RedirectNotFoundStorage
   

   lets add the backend_overridable tag here, see https://www.drupal.org/node/2306083

5. +++ b/modules/redirect_404/src/EventSubscriber/Redirect404Subscriber.php
   @@ -0,0 +1,94 @@
   +      if (!$this->configFactory->get('log_404')) {
   +        return;
   

   that means we don't need this check and also don't have to inject config.

6. +++ b/modules/redirect_404/src/Form/RedirectFix404Form.php
   @@ -0,0 +1,128 @@
   +      '#empty' => $this->config('redirect_404.settings')->get('log_404') ? $this->t('No 404 pages without redirects found.') : $this->t('404 requests are currently not logged, enable it in the Settings.'),
   

   this conditional can go away as well then, just use the first version.

7. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
   @@ -0,0 +1,79 @@
   +   * Gets the plain database query object for the redirect_404 table as a list.
   ...
   +   * @return \Drupal\Core\Database\Query\SelectInterface $query
   +   *   The plain result object of the redirect_404 table query.
   

   That's not true anymore.

8. +++ b/redirect.install
   @@ -45,7 +45,6 @@ function redirect_update_8100(&$sandbox) {
    }
    
   -
    /**
   

   unrelated change.

9. +++ b/redirect.module
   @@ -39,6 +39,11 @@ function redirect_hook_info() {
    
   +  // Add redirect_404_update onto the end of hooks[] if its module is enabled.
   +  if (\Drupal::moduleHandler()->moduleExists('redirect_404')) {
   +    array_push($hooks, 'redirect_404_update');
   +  }
   

   no idea what this is :) There is no such hook.

10. +++ b/redirect.services.yml
    @@ -13,7 +13,7 @@ services:
           - { name: event_subscriber }
       redirect.settings_cache_tag:
    -        class: Drupal\redirect\EventSubscriber\RedirectSettingsCacheTag
    -        arguments: ['@cache_tags.invalidator']
    -        tags:
    -          - { name: event_subscriber }
    +    class: Drupal\redirect\EventSubscriber\RedirectSettingsCacheTag
    +    arguments: ['@cache_tags.invalidator']
    +    tags:
    +      - { name: event_subscriber }
    diff --git a/src/Entity/Redirect.php b/src/Entity/Redirect.php
    

    Valid fix, but unrelated.

11. +++ b/src/Entity/Redirect.php
    @@ -1,8 +1,4 @@
     <?php
    -/**
    - * @file
    - * Contains \Drupal\redirect\Entity\Redirect.
    - */
    

    same

12. +++ b/src/Form/RedirectForm.php
    @@ -1,10 +1,5 @@
     
    -/**
    - * @file
    - * Contains \Drupal\redirect\Form\RedirectForm.
    - */
    -
    

    same. We should not have any changes anymore in redirect except removing code.

13. +++ b/src/Form/RedirectSettingsForm.php
    @@ -55,6 +50,18 @@ class RedirectSettingsForm extends ConfigFormBase {
    +
    +    // Add 404 checkbox to the settings form if redirect_404 module is enabled.
    +    if (\Drupal::moduleHandler()->moduleExists('redirect_404')) {
    

    this can go too.

All done.

+++ b/modules/redirect_404/src/Tests/Fix404RedirectUITest.php
@@ -0,0 +1,91 @@
+
+    // Check if we generate correct Add redirect url and if the form is
+    // pre-filled.
+    $destination = Url::fromUri('base:admin/config/search/redirect/404')->toString();
+    $this->assertUrl('admin/config/search/redirect/add', ['query' => ['path' => 'non-existing', 'langcode' => 'en', 'destination' => $destination]]);
+    $this->assertFieldByName('redirect_source[0][path]', 'non-existing');
+

You have those query arguments wrong.

See \Drupal\redirect\Form\RedirectForm::prepareEntity, you need to pass what's actually implemented. That means source and not path and language and not langcode.

Updated "path --> source" and "langcode --> language" in both redirect_404 tests and RedirectFix404Form, removed debugs previously put for testing and reverted some @file doc leftover (issue about Redirect cleanups will be committed here: #2717103: Removing @file docblocks).

Running Fix404RedirectUILanguageTest locally, there is still testAutomaticRedirects (this is in RedirectUITest) that fails at $this->repository->findMatchingRedirect('term_test_alias');, it complaints about $this->repository not being an object, but findMatchingRedirect() has been called several times before like that and there are problems only here (?)... Let's see what happens in testbot now...

Aaand here the missing patch...

Found the bug... Fixed.

Discussed with @miro_dietiker, we should drop the path that has already a redirect assigned form the Fix 404 pages list. Here I've just started adding the field to the redirect_404 table. Still work in progress...

I reviewed the situation and defined:
- We should clean up the table, similar to dblog, based on cron: remove records with smallest count and oldest hit
- 404's that are resolved (aka adding a redirect) should disappear. Thus we add a "resolved" column
-- Hitting a record that was previously marked as resolved, removes the flag again (happens when a redirect is deleted)
-- Thus we add a resolved column
- The admin UI should be built with a view
-- With language as an exposed filter
- Yeah, the UI should not link to an inexisting path
Thus, we have low complexity and the new 404 module is focussed to its purpose with a good experience.

+++ b/modules/redirect_404/src/Form/RedirectFix404Form.php
@@ -70,10 +70,13 @@
+      if ($result->resolved) {

Should use a view (see above) and should be excluded with the filter when doing the query.

Finally, we have identified some follow-ups that possibly need discussions first. Please create the follow-up issues when we are near completion.

We need to decide if we still want to maintain a non-views fallback like #2716883: Make views dependency optional
To avoid the double complexity, we usually drop it and depend to views...

Also, do we need / can we add an option to disable the regular 404 dblog records to avoid record duplication?

Follow-up: Drupal can operate multiple domains and for 404's it's important to also know about the domain it originated.
(Domains could also be bound to languages through language negotiation.)

After discussions, debug, fixing stuff, ... finally there is a patch...
What is done now:
2) 404's that are resolved (aka adding a redirect) should disappear.
-a- Hitting a record that was previously marked as resolved, removes the flag again (happens when a redirect is deleted)
-b- Thus we add a resolved column
4) Yeah, the UI should not link to an inexisting path

Next step, view (reason: more flexible custom modifications):
3) The admin UI should be built with a view
-a- With language as an exposed filter

As what we've discussed yesterday, this is part of the followup:
1) We should clean up the table, similar to dblog, based on cron: remove records with smallest count and oldest hit

Awesome progress and great to already have test coverage!

The views conversion is important for us, because we need capability to clone the view and offer a varying 404 tab with specific filters.

From reviewing the situation, i think the dblog cleanup with cron can not be a follow-up. Because currently dblog can be cleaned up and doesn't fill the DB.
But with the new module, the DB is filled more and more with every 404. That's a regression that could end up in a critical issue if bots are scanning your website with invalid URLs...

From reviewing the situation, i think the dblog cleanup with cron can not be a follow-up. Because currently dblog can be cleaned up and doesn't fill the DB.
But with the new module, the DB is filled more and more with every 404. That's a regression that could end up in a critical issue if bots are scanning your website with invalid URLs...

Agreed. Not just bots. It could also be used for a DoS attack.

The views conversion is important for us, because we need capability to clone the view and offer a varying 404 tab with specific filters.

@berdir mentioned in #23 that we might want to implement different backends (this is a perfect fit for Redis or MongoDB's capped collections). Views become a bit problematic in that case...

1. +++ b/modules/redirect_404/src/EventSubscriber/Redirect404Subscriber.php
   @@ -0,0 +1,80 @@
   +   * Constructs a new RedirectNotFoundStorage.
   

   Wrong class name in the comment.

2. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
   @@ -0,0 +1,101 @@
   +      ->fields([
   +        'timestamp' => REQUEST_TIME,
   +        'count' => 1,
   +        'resolved' => 0,
   +      ])
   

   Should we create an index on this? It should speed up writes, specially in case of large datasets.

3. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
   @@ -0,0 +1,101 @@
   +  public function updateLogRequest($path, $langcode) {
   

   This seems a bit unfortunate name of the function to me. It is not updating the log item but marking it as resolved.

4. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
   @@ -0,0 +1,101 @@
   +      ->orderByHeader($header)
   ...
   +    $results = $query->condition('resolved', 0, '=')->execute()->fetchAll();
   

   Should we index fields that can be used to sort? At least count, which is selected by default?

5. +++ b/modules/redirect_404/config/schema/redirect_404.schema.yml
   @@ -0,0 +1,9 @@
   +      label: '404 error database logs to keep'
   
   +++ b/modules/redirect_404/redirect_404.install
   @@ -0,0 +1,53 @@
   +    'primary key' => ['path', 'langcode'],
   

   .

6. +++ b/modules/redirect_404/src/EventSubscriber/Redirect404Subscriber.php
   @@ -0,0 +1,80 @@
   +   * Constructs a new RedirectNotFoundStorage.
   

   Wrong class name.

7. CodeSniffer reports some things:

   ➜  redirect git:(8.x-1.x) ✗ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,js,css,info,txt,md modules/redirect_404 
   
   FILE: ...croot/modules/redirect/modules/redirect_404/redirect_404.install
   ----------------------------------------------------------------------
   FOUND 1 ERROR AFFECTING 1 LINE
   ----------------------------------------------------------------------
    4 | ERROR | [x] The second line in the file doc comment must be
      |       |     "@file"
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   
   
   FILE: ...redirect/modules/redirect_404/src/Tests/Fix404RedirectUITest.php
   ----------------------------------------------------------------------
   FOUND 6 ERRORS AFFECTING 5 LINES
   ----------------------------------------------------------------------
     15 | ERROR | [ ] Missing short description in doc comment
     20 | ERROR | [ ] Missing short description in doc comment
     25 | ERROR | [ ] Missing short description in doc comment
     78 | ERROR | [ ] If the line declaring an array spans longer than
        |       |     80 characters, each element should be broken into
        |       |     its own line
     78 | ERROR | [ ] If the line declaring an array spans longer than
        |       |     80 characters, each element should be broken into
        |       |     its own line
    108 | ERROR | [x] Expected 1 newline at end of file; 2 found
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   
   
   FILE: .../modules/redirect_404/src/Tests/Fix404RedirectUILanguageTest.php
   ----------------------------------------------------------------------
   FOUND 5 ERRORS AFFECTING 4 LINES
   ----------------------------------------------------------------------
     19 | ERROR | [ ] Missing short description in doc comment
     60 | ERROR | [x] Missing function doc comment
     84 | ERROR | [ ] If the line declaring an array spans longer than
        |       |     80 characters, each element should be broken into
        |       |     its own line
     84 | ERROR | [ ] If the line declaring an array spans longer than
        |       |     80 characters, each element should be broken into
        |       |     its own line
    134 | ERROR | [x] Expected 1 newline at end of file; 2 found
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   
   
   FILE: ...es/redirect/modules/redirect_404/src/Form/RedirectFix404Form.php
   ----------------------------------------------------------------------
   FOUND 2 ERRORS AND 1 WARNING AFFECTING 3 LINES
   ----------------------------------------------------------------------
     8 | WARNING | [x] Unused use statement
    11 | ERROR   | [x] Missing class doc comment
    91 | ERROR   | [x] Expected 1 space after "=>"; 0 found
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   

Discussed about pluggability.
When looking at past, we did past as API module defining the logging interface and event capture and past_db that does the db storage, plus offering the UI.
Current code is simple and database centric. As long as we are using the DB, i think we should following the core pattern / direction of using views as much as possible.

Once generalisation is done, we can reintroduce all the complexity of pluggability and configurability. For the time bing i think the only possibly healthy piece would be to rely to a StorageInterface in the EventSubscriber and not the Storage itself.

Sorry for the late update, discussions and supports took me a while...

For now, cleanuped code pointed by @shlashrsm (1/6, 3, 7). 2 and 4 has been discussed: in our case here, it is important to write/store our records fast. indexes make it faster to search tables, but longer to write to. having unused indexes (we would use it only to easily sort and select the records to cleanup) will end causing some unnecessary slow down. thus to have a fast write operation, we should not have indexes here.
As discussed, we will need the view thing first, so next: views! :D

Tiny update: implemented a StorageInterface

So, I've tried to find out how to provide our redirect_404 fields and filters to use in a view, but still don't get where I need to say "use redirect_404 table" without an entity.
Currently there is only the Redirect view setting, so only the redirect table is accessible, and only User can be selected as a relationship, which cannot be used to reach our redirect_404 table.

For now I've just provided Path and Language field/filter, and Add Redirect button field plugins, and a ViewsData class.
I need some help to continue working on this, but let's discussed it tomorrow :)

Found it! Now I'm using a hook_views_data() and a wizard plugin to provide our fields and filters. Still work in progress, but here there is some first steps (not clean version).
Now I'm focusing on the operation buttons.

Maybe already one question: how can I convert a langcode (from the db) to a language to be displayed in the view ?

Yay! So now we have a nice view I think :D
Still need to clean up some code and add a test for our view filters. And the "dblog cleanups".

Looks really nice. Some minor feedback.

1. +++ b/modules/redirect_404/src/Plugin/views/field/Language.php
   @@ -0,0 +1,43 @@
   +class Language extends LanguageField {
   

   I'm irritated, there is a language column in admin/content and core should provide a field handler already.

2. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
   @@ -0,0 +1,78 @@
   +      $wildcard = '%' . trim(preg_replace('!\*+!', '%', $this->database->escapeLike($search)), '%') . '%';
   +      $query->condition('path', $wildcard, 'LIKE');
   

   This looks non-D8-alike and risky. Plz recheck for a nice pattern.

   What irritates me is that preg_replace() seems to get wrong parameters. Does this do anything real?

1. +++ b/modules/redirect_404/redirect_404.views.inc
   @@ -0,0 +1,119 @@
   +    'field' => '',
   +    'title' => t('Something'),
   +    'help' => t('Contains a list of something.'),
   

   It needs to be populated.

2. +++ b/modules/redirect_404/src/Plugin/views/field/AddRedirect.php
   @@ -0,0 +1,70 @@
   +    if (\Drupal::entityTypeManager()->getAccessControlHandler('redirect')->createAccess()) {
   ...
   +    return \Drupal::service('renderer')->render($operations);
   

   Inject.

3. +++ b/modules/redirect_404/src/Plugin/views/field/Language.php
   @@ -0,0 +1,43 @@
   +  public function query() {
   +    $this->ensureMyTable();
   +
   +    $fields = [
   +      'redirect_404_langcode' => [
   +        'table' => 'redirect_404',
   +        'field' => 'langcode'
   +      ],
   +    ];
   +    $this->addAdditionalFields($fields);
   +  }
   

   This shouldn't be needed. Since the key in views data matches the name of the column in DB it should work automatically.

4. +++ b/modules/redirect_404/src/Plugin/views/field/Language.php
   @@ -0,0 +1,43 @@
   +    $langcode = $this->getValue($values, 'redirect_404_langcode');
   

   You don't need to provide field if you're getting the default one.

5. +++ b/modules/redirect_404/src/Plugin/views/field/Language.php
   @@ -0,0 +1,43 @@
   +    $language = ConfigurableLanguage::load($langcode)->label();
   

   ConfigurableLanguage is in language module. We either need to specify it as a dependency or find a way to make it optional. Maybe display a raw langcode if class does not exist?

6. +++ b/modules/redirect_404/src/Plugin/views/wizard/Redirect404.php
   @@ -0,0 +1,37 @@
   +/**
   + * Defines a wizard for the watchdog table.
   + *
   + * @ViewsWizard(
   + *   id = "redirect_404",
   + *   module = "redirect_404",
   + *   base_table = "redirect_404",
   + *   title = @Translation("Redirect 404")
   + * )
   + */
   +class Redirect404 extends WizardPluginBase {
   

   Wrong comment.

7. +++ b/modules/redirect_404/src/RedirectNotFoundStorageInterface.php
   @@ -0,0 +1,48 @@
   +/**
   + * Defines a common interface for redirect 404 classes.
   + */
   +interface RedirectNotFoundStorageInterface {
   

   Comment seems wrong.

8. +++ b/src/Form/RedirectForm.php
   @@ -142,6 +142,14 @@ class RedirectForm extends ContentEntityForm {
   +    // Update 'resolved' field in redirect_404 table, if its module is enabled.
   +    $path = '/' . $this->requestStack->getCurrentRequest()->query->get('source');
   +    $langcode = $this->requestStack->getCurrentRequest()->query->get('language');
   

   Shouldn't we get those two from the redirect entity?

Done points from #63.1-7.
#63.8 I need to do it like that because otherwise we will get the added redirect selected language instead of the original recorded one, which needs to match in the db.

Still need to do points of #62 after lunch and add tests.

Added view to be installed when the module is enabled, added test for Path and Language filter (not working now), and cleanup a bit of code. Will continue next week :)

Added small feature (in the 404 page, display only the languages of the paths that are still resolved = 0), improved the tests.

Added cron job to clean up redirect_404 rows (temporary set row_limit to 3 for test).

About the failing tests, why is testbot getting these /checkout from Url::fromUri('base:admin/config/search/redirect/404')->toString() ?

Just a partial quick review.

1. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,90 @@
   +function _redirect_404_get_langcodes() {
   +  $results = db_query('SELECT DISTINCT(langcode) FROM {redirect_404} WHERE resolved = :resolved ORDER BY langcode', ['resolved' => 0])
   
   +++ b/modules/redirect_404/redirect_404.views.inc
   @@ -0,0 +1,92 @@
   +      'options callback' => '_redirect_404_get_langcodes',
   

   This is very expensive. Note that we don't have a langcode index.
   With views, it's common that options display ALL possible values and not just what has data. If you want this, you use something like facets and an index that can power it efficiently.
   Nice idea, but let's switch to just list all languages.

2. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,90 @@
   +      ->orderBy('count', 'DESC')
   +      ->orderBy('timestamp', 'DESC')
   ...
   +      \Drupal::database()->delete('redirect_404')
   +        ->condition('timestamp', $query, '<')
   +        ->execute();
   

   This doesn't work.

   Say the limit is 10 and 9 items have a count >1 and the 10th item has just the most recent hit with a count = 1. You would delete ALL data.

3. +++ b/modules/redirect_404/src/Tests/Fix404RedirectUILanguageTest.php
   @@ -0,0 +1,201 @@
   +        'destination' => $destination,//ltrim($destination, '/'),
   

   hmm? and there are many similar commented rows. :-)

1. reverted condition and fixed the test.
2. I was about to follow your hint but they said that in MySQL is not allowed to delete the table used in a subquery, so I thought to write something like that, but sure it is not fine...
3. From tests fail in #68 I thought it was because of that, so I've tried to just give the $destination without any ltrim() but apparently is not that either... reverted. I guess it will still fail :/

+++ b/modules/redirect_404/redirect_404.module
@@ -0,0 +1,90 @@
+ */
+function redirect_404_cron() {
+  // Cleanup the redirect_404 table.
+  $row_limit = \Drupal::config('redirect_404.settings')->get('row_limit');
+
+  // For row limit n, get the wid of the nth row in descending wid order.
+  // Counting the most recent n rows avoids issues with wid number sequences,
+  // e.g. auto_increment value > 1 or rows deleted directly from the table.
+  if ($row_limit > 0) {
+    $query = \Drupal::database()->select('redirect_404', 'r404')
+      ->extend('Drupal\Core\Database\Query\TableSortExtender')
+      ->orderBy('count', 'DESC')
+      ->orderBy('timestamp', 'DESC')
+      ->extend('Drupal\Core\Database\Query\PagerSelectExtender')
+      ->limit($row_limit - 1)
+      ->fields('r404', ['timestamp'])
+      ->execute()->fetchField();
+
+    // Delete all table entries older than the nth row, if nth row was found.
+    if ($query) {
+      \Drupal::database()->delete('redirect_404')
+        ->condition('timestamp', $query, '<')
+        ->execute();

I guess this is taken from dblog_cron(), which has a unique id which works better there.

Adding such an id would not help us, since we update records and don't have a strict order based on the ID.

Instead of row count, we could instead define an age and then delete records that were not accessed anymore since N days. Could still fill the logs if you do a huge a mount of requests over a short time, but the data we write does not use a lot of space and you'll have other problems too if you have such a high amount of requests over a short time?

That said, to be honest, I really don't care if we delete a bunch of entries a second too late or too early.

However, the logic needs to be a method in the storage service (purgeOld() or so)

About the views, views has a system for preventing to install default config if the storage for it is not present. See \Drupal\views\Entity\View::isInstallable(), if the views data is not there, the view is not installed. So what you can do is make sure that you only define your views data when the service is using our specific implementation (\Drupal::service('...') instanceof OurClassName).

I'm closed to the solution, but there is still something trivial that leads me to more complex situation...

Since we've tried with
delete from redirect_404 where (path, langcode) in (select path, langcode from redirect_404 order by count desc, timestamp desc limit 2, 1);
and got the error
Error This version of MySQL doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery’
so we've ended up to something that would simplify how to sort the rows and drop the irrelevant ones.

The idea now is to provide an extra field, relevancy, which value is updated based on the visit frequency of a redirect_404 record (radioactivity exponential decay concept). Given the formula $new_energy = $old_energy * $decay and $decay = pow(2, - $elapsed / $halflife) (N(t) = N_0 * e^(-t/T)), in our case we have:

$elapsed: REQUESTED_TIME - (last timestamp stored for this request)
$halflife: i've set it as a day per seconds (60*60*24 = 86400), otherwise if it is smaller it decay faster, but this is tbd

We thought that this $decay should be multiplied to the relevancy of the requested record, but we want to "downgrade" all the other to "give" more relevance to this requested row, so:

$old_energy: last relevancy stored for all the relevancy != this request (1: max relevancy)

I've tried to do this in a subquery and get that value to update all the other rows in the Sequel Pro console:

update redirect_404 set relevancy=(
  relevancy*(select pow(2, -(UNIX_TIMESTAMP(NOW()) - IFNULL(timestamp, UNIX_TIMESTAMP(NOW())))/86400 ) as decay
  from redirect_404
  where path='/test0' and langcode='en'))
where timestamp NOT IN
(select timestamp from redirect_404 where path='/test0' and langcode='en');

but it is not working (same as delete): You can't specify target table 'redirect_404' for update in FROM clause...

At the moment the uploading interdiff has only the new field schema and some attempts in RedirectNotFoundStorage::logRequest() to store/update relevancy. Once this works I'll move the cleanup logic in this service.

About the view, how/where should I create that method/check without the entity ?

Other possible way, something with MySQL triggers ?

I think you're overcomplicating this. I'll repeat my comment from above:

That said, to be honest, I really don't care if we delete a bunch of entries a second too late or too early.

What you are describing sounds like a ton of complexity, slower writes on 404 requests and more complicated delete queries.

IMHO, your only problem in the first attempt was to sort by count first. Just don't do that. Get the oldest timestamp (timestamp is updated on each new request) after the amount of entries that should be kept, then delete everything of the same age and older.

AFAICS, there are only two problems with that:

1. We might keep 1003 records instead of the configured 1000 because record 1000-1003 happend in the same second. So what? On a site that is that busy, there are likely going to be new requests while we're doing this delete, so it will already have more anyway. Doesn't matter.
2. On a very busy site, with thousands of unique 404 requests, more important 404's with multiple requests might get deleted. I expect we'll set the default to 10k or so. That means more than 10k requests on different 404's need to happen to delete data for such a case. That's quite a lot, and if you're worried about that, just set it to 100k or so. We're storing very little data, much less than a watchdog entry, so storing 10 or 100k entries really isn't an issue.

Yeah, thx for trying to move forward with this idea @tduong.

Not sure if the energy / relevancy pattern is a good idea. I was just thinking out loud.
Here's the missing piece: The relevancy value is only updated when the row is updated. Thus, the relevancy stored is the relevancy at the time of the timestamp.

With this pattern, you usually never need to do bulk updates to those tables.
Instead you should order by calculating the current relevancy, something like:
ORDER BY pow(2, -(UNIX_TIMESTAMP(NOW()) - timestamp))/86400 DESC

IMHO the complexity looks pretty limited with this approach and we can filter out irrelevant things easily.
All other tries failed. We are open for better approaches.

(We are only using the DB to store raw data and have business logic in PHP. Triggers and stored procedures is not how Drupal usually does it. Also i don't see how it could simplify things here... ;-) )

That was a cross post with Berdir.

Note that when a bot like Google scans your site, every 404 page is receiving a single hit in a period.
If you have these kind of bots active and still over time cumulated items with currently very low frequency, you risk to loose them.
I agree to simplify, we can cutoff based on large limit numbers with timestamp order.
I discussed with tramy to figure out how much we can simplify the approach and if it's worth considering. If it's complicated it's not and we most likely can live with the limitation.

Couldn't make it work even with two queries. Tomorrow I will try with the timestamp and make it work.

Finally it works, but only using query() instead of select() and delete(). Still need to clean up some code and move the cron logic in the storage service, after lunch :P

Uh, something to mention: the relevancy values are stored wrong now because the most 404 page visited is *decreasing* instead of increasing, but I don't think that doing $relevancy = $relevancy + $relevancy * $decay is a good solution because it grows too fast (?)

And also in the views.view.redirect_404.yml I've commented some lines because it breaks the view, but as soon as someone export the view, he/she gets these lines enabled again... Not sure how to fix it (count numeric in views data) and if doing what @Berdir suggested above will avoid this bug...

Yeah we missed the hit +1 here:
$relevancy = $relevancy * $decay + 1

Some quick reviews again. :-)

1. +++ b/modules/redirect_404/config/install/redirect_404.settings.yml
   @@ -0,0 +1 @@
   +row_limit: 3
   

   I think we should ship a better default. :-)

2. +++ b/modules/redirect_404/config/install/views.view.redirect_404.yml
   @@ -0,0 +1,533 @@
   +          content: 'No 404 pages without redirects found.'
   

   Double negation makes my mind melt. How about "There are no 404 errors to fix."

3. +++ b/modules/redirect_404/redirect_404.install
   @@ -0,0 +1,60 @@
   +        'not null' => TRUE,
   +        'description' => 'The path of the request',
   ...
   +        'description' => 'The language of this request',
   ...
   +        'not null' => TRUE,
   

   I would prefer a standard order of the keys.

4. +++ b/modules/redirect_404/redirect_404.install
   @@ -0,0 +1,60 @@
   +        'description' => 'The number of requests with that path and language',
   

   (multiple) Missing final dots in description in schema.

5. +++ b/modules/redirect_404/redirect_404.views.inc
   @@ -0,0 +1,92 @@
   +      'options callback' => '_redirect_404_get_langcodes',
   

   Back again? Thought you confirmed removal of the language distinct query?

6. +++ b/modules/redirect_404/src/EventSubscriber/Redirect404Subscriber.php
   @@ -0,0 +1,81 @@
   +    debug($this->redirectStorage->purgeOldRequests());
   

   We should not purge on exception. And don't do a debug either. :-)

7. +++ b/modules/redirect_404/src/Form/RedirectFix404Form.php
   @@ -0,0 +1,130 @@
   +class RedirectFix404Form extends FormBase {
   

   So we drop this whole class finally?
   (otherwise view would need to go into optional...)
   Please don't forget to add the dependency to views!

Oook, now there is something that works! :D

Discussions, several attempts and then finally fixed the cleanup cronjob, cleaned up some code, refactored points mentioned above.
About 6., it was just for me, to test/see the result (debug is too slow), and I've uploaded the patch in case there was someone that wanted to review it :P
Point 7., now I've added a @todo for that form and listRequest(). We will discuss it later or in a follow up :)
For now I still have the view in install. Didn't check if the tests pass if it is in /optional. I'll check it tomorrow.

Unfortunately there is something wrong in the KernelTest that I've just written after the cronjob fix: somehow out purgeOldRequests() method in redirect_404_cron() is not triggered. Anyone knows why and how to make it work ?

(...go to watch the match :D)

+++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
@@ -0,0 +1,104 @@
+    $select = $this->database->query('SELECT path, langcode, (relevancy * pow(2, -(UNIX_TIMESTAMP(NOW()) - timestamp)/' . $this->halflife . ')) as cutoff FROM {redirect_404} ORDER BY (relevancy * pow(2, -(UNIX_TIMESTAMP(NOW()) - timestamp)/' . $this->halflife . ')) DESC LIMIT ' . $row_limit . ', 1')->fetchObject();
...
+    $this->database->query('DELETE FROM {redirect_404} WHERE (relevancy * pow(2, -(UNIX_TIMESTAMP(NOW()) - timestamp)/' . $this->halflife . ')) < :cutoff', [':cutoff' => $select->cutoff]);

If the table is empty or even if there are less records than limit, this looks to me like it will fail fatal. Also $select seems a bad name for a row - plus neither path nor langcode are needed here.

IMHO the added complexity by this pattern is minimalistic. The functional part is 2 lines of code compared to timestamp sorting (plus schema lines). But we should add comments about the math so people can understand it. :-)

#97: again, it was just for testing as a "mid-solution". Sorry for uploading dirty patches ^^'

I'll post a comment with the explanation first, so next the uploading patch will be on the top of the list in the IS.
Now the Redirect 404 view works fine, and also the filters (our Language filter takes the default Drupal language filter) (see screenshots "Fix 404 pages view" and "view filters"), after assigning a redirect to a record ("add redirect") it appears in the Redirect page ("redirect"), if you look for that path in the Fix 404 page it is not displayed anymore since it is marked as resolved ("redirect assigned").

Mysql math performance information:
We've tested with ~400'000 rows through drush sql-cli command line.
The time it took to run the select query to order by the effective relevancy at the current time was 2.69 sec (the same query with the limit condition to get the cutoff value should last more or less the same because it has to run through every row to compute the math expression anyway but it doesn't have to display each rows, so we got 1.09 sec for this), then for the delete query it took 3.06 sec (see the first 2 screenshots).

Now this patch should be clean enough :P
- moved views.view.redirect_404.yml in /optional, and added check in redirect_404_views_data() to only define our views data when the service is using our specific implementation
- cleaned up some codes (in RedirectFix404Form there are only array syntax refactoring)
- added comment/math explanation in the RedirectNotFoundStorage doc block, and some further comments in some methods and test
- cleaned/shortened/break in multiple lines some queries that were too long (hope it doesn't hurt the readability too much ^^')
- fixed web and kernel tests

1. +++ b/modules/redirect_404/config/install/redirect_404.settings.yml
   new file mode 100644
   index 0000000..d86d593
   
   index 0000000..d86d593
   --- /dev/null
   
   --- /dev/null
   +++ b/modules/redirect_404/config/optional/views.view.redirect_404.yml
   

   it doesn't have to be optional config. there are no additional dependences, so it can't ever not be installed, except when the storage is not there, but that happens on a different level.

2. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,52 @@
   +/**
   + * @file
   + * Clean redirect_404 from irrelevant rows to keep a limited amount of records.
   + */
   

   That's not the description of the file but from the purge old requests method.

   This should simply say something like "Module file for redirect_404."

3. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,52 @@
   +/**
   + * @defgroup redirect_api Redirection API
   + * @{
   + * Functions related to URL redirects.
   + *
   + * @} End of "defgroup redirect_api".
   + */
   

   remove this.

4. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,52 @@
   +function redirect_404_form_system_logging_settings_alter(&$form, FormStateInterface $form_state) {
   +  $row_limits = [100, 1000, 10000, 100000, 1000000];
   +  $form['dblog_row_limit'] = [
   +    '#type' => 'select',
   +    '#title' => t('404 error database logs to keep'),
   +    '#default_value' => \Drupal::configFactory()->getEditable('redirect_404.settings')->get('row_limit'),
   +    '#options' => [0 => t('All')] + array_combine($row_limits, $row_limits),
   +    '#description' => t('The maximum number of 404 error logs to keep in the database log. Requires a <a href=":cron">cron maintenance task</a>.', [':cron' => \Drupal::url('system.status')])
   +  ];
   +
   +  $form['#submit'][] = 'redirect_404_logging_settings_submit';
   

   wondering what makes more sense, having this in the redirect settings or in the loggin settings. Not sure...

5. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,52 @@
   +  \Drupal::configFactory()->getEditable('redirect_404.settings')->set('row_limit', $form_state->getValue('dblog_row_limit'))->save();
   

   use chaining to make this more readable

   \Drupal::configFactory()
   ->getEditable()
   ->set()
   ->save()

6. +++ b/modules/redirect_404/redirect_404.routing.yml
   @@ -0,0 +1,7 @@
   +redirect.fix_404:
   +  path: '/admin/config/search/redirect/404'
   +  defaults:
   +    _title: 'Fix 404 pages'
   +    _form: '\Drupal\redirect_404\Form\RedirectFix404Form'
   +  requirements:
   +    _permission: 'administer redirects'
   

   not sure if we really need bot the view and the controller?

7. +++ b/modules/redirect_404/redirect_404.views.inc
   @@ -0,0 +1,96 @@
   +  $data = [];
   +  if (!\Drupal::service('redirect.not_found_storage') instanceof RedirectNotFoundStorage) {
   +    return $data;
   

   a comment for this would be good.

8. +++ b/modules/redirect_404/src/Form/RedirectFix404Form.php
   @@ -0,0 +1,135 @@
   +/**
   + * Provides a form that lists all 404 error paths and no redirect assigned yet.
   + *
   + * Keep this form, if we want to implement different backends to better fit
   + * Redis or MongoDB's capped collections. Otherwise drop this class and the
   + * RedirectNotFoundStorage::listRequests() method. See discussion in #1559310
   + * comment #55.
   + */
   +class RedirectFix404Form extends FormBase {
   

   ah, it was discussed here.

   I would avoid "we", try to write it from a third person (In case someone uses an alternative backend).

9. +++ b/modules/redirect_404/src/Plugin/views/field/AddRedirect.php
   @@ -0,0 +1,120 @@
   +  public function render(ResultRow $values) {
   +    $link = [];
   +    if ($this->entityTypeManager->getAccessControlHandler('redirect')->createAccess()) {
   +      $query = [
   +        'query' => [
   +          'source' => ltrim($this->getValue($values, 'redirect_404_path'), '/'),
   +          'language' => $this->getValue($values, 'redirect_404_langcode'),
   

   field plugins have a special access method, then it won't show up at all, this would give a column with empty and possibly broken operations.

10. +++ b/modules/redirect_404/src/Plugin/views/field/Language.php
    @@ -0,0 +1,28 @@
    +  public function render(ResultRow $values) {
    +    $langcode = $this->getValue($values);
    +    $language = ConfigurableLanguage::load($langcode)->label();
    +
    

    You should use getLanguage() from the language manager. otherwise you might get problems with UND or deleted languages. Even then, make sure you really get a language object back, in case a language gets deleted.

11. +++ b/modules/redirect_404/src/Plugin/views/wizard/Redirect404.php
    @@ -0,0 +1,32 @@
    + */
    +class Redirect404 extends WizardPluginBase {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  protected function defaultDisplayOptions() {
    +    $display_options = parent::defaultDisplayOptions();
    +
    +    // Add permission-based access control.
    +    $display_options['access']['type'] = 'perm';
    +    $display_options['access']['options']['perm'] = 'administer redirects';
    +
    +    return $display_options;
    +  }
    

    not sure if having a wizard for this is worth it, only very, very few people will create additional new views.

12. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
    @@ -0,0 +1,120 @@
    +/**
    + * Controller class for redirect not found storage.
    + *
    

    this is not a controller: Sql implementation for ...

    Might make sense to also include that part in the class name: SqlRedirectNotFoundStorage

13. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
    @@ -0,0 +1,120 @@
    +    $cutoff_exp = '(relevancy * pow(2, -(UNIX_TIMESTAMP(NOW()) - timestamp)/86400.00))';
    +    $cutoff = $this->database
    +      ->query('SELECT ' . $cutoff_exp . ' as cutoff FROM {redirect_404} ORDER BY ' . $cutoff_exp . ' DESC LIMIT ' . $row_limit . ', 1')
    +      ->fetchField();
    

    does ORDER BY cutoff not work?

    limit like this is not standard compliant I think.

    it would be better to use a select() query with addExpression()

14. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
    @@ -0,0 +1,120 @@
    +    // Delete records below cutoff value, if given. Otherwise skip the cleanup.
    +    if (!empty($cutoff)) {
    +      $this->database
    +        ->query('DELETE FROM {redirect_404} WHERE ' . $cutoff_exp . ' < ' . $cutoff);
    +    }
    

    same here, even though it is safe here, never put dynamic variables directly into queries if it can be avoided. this should be easy with ->delete() and a condition?

15. +++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
    @@ -0,0 +1,120 @@
    +      // Replace wildcards with PDO wildcards.
    +      // @todo Find a way to write a nicer pattern, if we want to implement
    +      // different backends to better fit Redis or MongoDB's capped collection,
    +      // thus keep Drupal\redirect_404\Form\RedirectFix404Form. Otherwise drop
    +      // the form and this method. See discussion in #1559310 comment #55.
    +      $wildcard = '%' . trim(preg_replace('!\*+!', '%', $this->database->escapeLike($search)), '%') . '%';
    

    a nicer pattern for what?

    comment seems very repetitive to the one on the class and doesn't tell me much. This pattern was used in 7.x as well, not sure why we have to replace it or how that is related to redis/mongodb.

16. +++ b/modules/redirect_404/src/RedirectNotFoundStorageInterface.php
    @@ -0,0 +1,54 @@
    +   * @param string $search
    +   *   The search text.
    

    this should contain documentation about which characters are expected to be treated as wildcards.

17. +++ b/modules/redirect_404/tests/src/Kernel/Fix404RedirectCronJobTest.php
    @@ -0,0 +1,55 @@
    +    // Change timestamp to 1000000 more for the first 6 rows to get different
    +    // relevancy so we won't delete all rows when we run cron.
    +    db_query("UPDATE {redirect_404} SET timestamp='1466082407' WHERE path BETWEEN '/test0' AND '/test5';");
    +    redirect_404_cron();
    

    queries should not have ; at the end.

    this should use db_update(), and this doesn't seem to be a sufficient test for the relevancy stuff.

    I'd expect to see a test that simulates a situtation where a record with a higher count and older timestamp would get deleted first, but the relevancy prevents that.

    Also the between check is too much magic for me, I'd expect the scenario above will require multiple, specific update queries (one with count 1/relevancy NOW() - 600 and timestamp x, one with count 5 and timestamp xy - 1200 and so on.

18. +++ b/modules/redirect_404/tests/src/Kernel/Fix404RedirectCronJobTest.php
    @@ -0,0 +1,55 @@
    +    // Check that now we have 6 rows in the redirect_404 table.
    +    $result = db_query("SELECT COUNT(*) as rows FROM {redirect_404};")->fetchObject();
    +    $this->assertEquals(6, $result->rows);
    

    you can use fetchField to get a single value.

19. +++ b/src/Form/RedirectForm.php
    @@ -142,6 +142,14 @@ class RedirectForm extends ContentEntityForm {
       public function save(array $form, FormStateInterface $form_state) {
    +    // Update 'resolved' field in redirect_404 table, if its module is enabled.
    +    $path = '/' . $this->requestStack->getCurrentRequest()->query->get('source');
    +    $langcode = $this->requestStack->getCurrentRequest()->query->get('language');
    +    $redirect = $form_state->getValue(['redirect_redirect', 0]);
    +    if (\Drupal::moduleHandler()->moduleExists('redirect_404') && !empty($redirect['uri'])) {
    +      \Drupal::service('redirect.not_found_storage')->resolveLogRequest($path, $langcode);
    +    }
    +
    

    might be possible to do this in redirect_404 instead. redirects are an entity which trigger an hook_redirect_insert() hook. you can listen to that and then call this there.

Points 1-3, 5-10, 12, and 18 done. Further feedback:
4: if we want to move this into redirect, then the not found storage service needs to be moved too, otherwise if the user does not enable the redirect_404 sub-module the service call in hook_cron() won't work, will it ? I guess it is fine to leave it there.

11: not fully sure how this magic works, but afaik without this wizard plugin we don't point the queries to any table. after past discussions with @miro_dietiker, he said we don't need an entity for redirect_404, so I'm not sure where else we can tell our view to look into the redirect_404 table.

15: from @miro_dietiker comment #62

+++ b/modules/redirect_404/src/RedirectNotFoundStorage.php
@@ -0,0 +1,78 @@
+      $wildcard = '%' . trim(preg_replace('!\*+!', '%', $this->database->escapeLike($search)), '%') . '%';
+      $query->condition('path', $wildcard, 'LIKE');

This looks non-D8-alike and risky. Plz recheck for a nice pattern.

What irritates me is that preg_replace() seems to get wrong parameters. Does this do anything real?

19: how ?

Currently doing: 13-17

Misread point 4. forget about what i wrote above.
Otherwise is there already any decision about what to do for these points left ?

In this patch:
13.-14. edited queries.
16. added comment for wildcard, but i guess i still need to improve it (?)
17. done. about the extended test, i've just updated the relevancy field in one line and afaik a test should not have too many randomised values, otherwise it hurts readability (?) also added helper methods to specifically assert which rows are there and which are not. also added the $resolved parameter in case we will need it for other tests in the future.
19. hook added, fixed related code.

Also noticed:
Instead of getting the path with $this->currentPath, we should use $this->requestStack to allow storing the arguments as well (tested with /foo?foo=1 and /foo?foo=2, with the previous code only /foo is stored for both cases), but now if we have /de/foo?foo=3, it stores the langcode correctly, but in path it is stored as /de/foo?foo=3. I will check/fix it tomorrow.

Uploading patch:

1. Improved how to get the langcode from the redirect_404_redirect_presave(): when a redirect has been assigned, in case you had a 404 path in 'en' and you want the redirect to be in 'de', if you use the redirect entity to get the langcode you will get 'de', but you want 'en' because you ve stored that path in 'en'. with the previous code then you wont be able to mark this path to resovled because "un-matching langcode". changed it.
2. After discussion with @Berdir, instead of just using $this->currentPath or $this->requestStack, we've combined both of them to get the cleaned path and append the (evtl.) URL's arguments.
3. Fixed the UItests, improved and extended them to cover also paths with URL arguments.
4. Improved the kerneltest to actually test the "order by" hardcoding every row's relevancy

Discussed with @Berdir about #106.1: redirecting to another language does not resolve the path. if the user wants to have a redirect to another language, that should be up to the user, not our problem. thus reverted it and fixed the Fix404RedirectUILanguageTest.

About the failing tests, calling $redirect->getSourceUrl() then it calls toString() which generates a different Url string for testbot. now this should be fixed (added a new method in Redirect.php to get the source url with its arguments), but i leave the debug statements in the test to evtl. see what else is the bug. thus, this is a dirty patch!

Refactored Redirect404Subscriber::onKernelException() to the D8 std.

Forgot to fixed a test line.

Great! Now i can upload a clean patch! :D

Last round of cleanup.

Lets discuss this first.

1. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,60 @@
   +  $form['dblog_row_limit'] = [
   +    '#type' => 'select',
   +    '#title' => t('404 error database logs to keep'),
   +    '#default_value' => \Drupal::configFactory()->getEditable('redirect_404.settings')->get('row_limit'),
   +    '#options' => [0 => t('All')] + array_combine($row_limits, $row_limits),
   +    '#description' => t('The maximum number of 404 error logs to keep in the database log. Requires a <a href=":cron">cron maintenance task</a>.', [':cron' => \Drupal::url('system.status')])
   +  ];
   

   dblog_row_limit is a weird name, has nothing to do with dblog. Should be redirect_404_row_limit.

   Also, we still need to discuss if this should be in the redirect settings or this form.

2. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,60 @@
   +  // Get the original langcode of this path, not the redirect one.
   +  $langcode = $redirect->get('language')->value;
   

   wrong comment, no longer true. Just remove it.

3. +++ b/modules/redirect_404/redirect_404.module
   @@ -0,0 +1,60 @@
   +
   +  // Update 'resolved' field in redirect_404 table.
   +  \Drupal::service('redirect.not_found_storage')->resolveLogRequest($path, $langcode);
   

   Too much technical details. It's a service, someone could write a service that uses redis as storage. Then there is no table.

   Suggestion: Mark a potentially existing log entry for this path as resolved.

4. +++ b/modules/redirect_404/redirect_404.views.inc
   @@ -0,0 +1,98 @@
   +
   +  $data['redirect_404']['table']['group'] = t('Redirect 404');
   +  $data['redirect_404']['table']['wizard_id'] = 'redirect_404';
   

   Lets look at this wizard thing together. There has to be a better way IMHO.

   I don't see one in statistics_views_data() for example

5. +++ b/modules/redirect_404/src/EventSubscriber/Redirect404Subscriber.php
   @@ -0,0 +1,96 @@
   +    // Log 404 only exceptions.
   +    if ($event->getException() instanceof NotFoundHttpException) {
   

   That doesn't sound like proper english:

   Only log page not found (404) errors.

6. +++ b/modules/redirect_404/src/Form/RedirectFix404Form.php
   @@ -0,0 +1,135 @@
   + * Keep this form, if they want to implement different backends to better fit
   + * Redis or MongoDB's capped collections. Otherwise drop this class and the
   + * SqlRedirectNotFoundStorage::listRequests() method. See discussion in
   + * #1559310 comment #55.
   + */
   +class RedirectFix404Form extends FormBase {
   

   I still don't think this comment will work.

   Otherwise to what? Either we actually have a @todo to do something, with an issue. Or we simply document it as a matter of fact.

   Which would be something like this:

   This is a fallback for the provided default view.

7. +++ b/modules/redirect_404/src/Form/RedirectFix404Form.php
   @@ -0,0 +1,135 @@
   +    if ($multilingual) {
   +      $header[] = ['data' => $this->t('Language'), 'field' => 'language'];
   +    }
   

   This is something that the view won't do yet, correct?

   We can actually make it, by implementing access() with the same logic.

8. +++ b/modules/redirect_404/src/Plugin/views/field/AddRedirect.php
   @@ -0,0 +1,126 @@
   + * Provides a views field for the 'Add redirect' button.
   + *
   + * @ingroup views_field_handlers
   + *
   + * @ViewsField("add_redirect")
   + */
   +class AddRedirect extends FieldPluginBase {
   

   We discussed a second operation for later: Manually resolve/ignore a path.

   So, lets rename this to RedirectOperations (also for machine names, keys, ..), then we can easily add more operations later.

9. +++ b/modules/redirect_404/src/Plugin/views/field/AddRedirect.php
   @@ -0,0 +1,126 @@
   +  public function query() {
   +    $this->ensureMyTable();
   +
   +    $fields = [
   +      'redirect_404_langcode' => [
   +        'table' => 'redirect_404',
   +        'field' => 'langcode'
   +      ],
   +      'redirect_404_path' => [
   +        'table' => 'redirect_404',
   +        'field' => 'path'
   +      ]
   +    ];
   +    $this->addAdditionalFields($fields);
   

   Have a look at \Drupal\views\Plugin\views\field\FieldPluginBase::init. I think we can put this into the views data definition, then we don't need this method.

10. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorageInterface.php
    @@ -0,0 +1,54 @@
    +/**
    + * Interface for redirect 404 services.
    + */
    +interface SqlRedirectNotFoundStorageInterface {
    

    The interface is *not* SQL specific. Only our implementation is. So.. no Sql prefix.

11. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorageInterface.php
    @@ -0,0 +1,54 @@
    +  /**
    +   * Cleans the irrelevant data stored in the redirect_404 table.
    +   */
    +  public function purgeOldRequests();
    

    Same here and in other cases here. As far as the interface is concerned, there is no redirect_404 table. Use a generic description instead, like 404 requests log.

12. +++ b/modules/redirect_404/src/Tests/Fix404RedirectUILanguageTest.php
    @@ -0,0 +1,222 @@
    +    // Add predefined language.
    +    $this->drupalPostForm('admin/config/regional/language/add', ['predefined_langcode' => 'fr'], t('Add language'));
    +    $this->assertText('French');
    

    we add french here and the other languages above?

13. +++ b/modules/redirect_404/src/Tests/Fix404RedirectUILanguageTest.php
    @@ -0,0 +1,222 @@
    +    $body = '/html/body/div/main/div/div/div/div/div[2]/table/tbody';
    +    $this->assertTrue(strpos($this->xpath($body)[0]->asXML(), 'French'));
    

    there is no reason to specify every single tag here. It will break for no reason. Especially since all you do then is just look for "French" in the whole table?

    What is that for exactly? If that's all you do, why not just do an assertText()?

14. +++ b/modules/redirect_404/src/Tests/Fix404RedirectUILanguageTest.php
    @@ -0,0 +1,222 @@
    +    $this->assertOption('edit-langcode', 'All');
    +    $this->assertOption('edit-langcode', '***LANGUAGE_site_default***');
    +    $this->assertOption('edit-langcode', '***LANGUAGE_language_interface***');
    +    $this->assertOption('edit-langcode', 'en');
    +    $this->assertOption('edit-langcode', 'de');
    +    $this->assertOption('edit-langcode', 'es');
    +    $this->assertOption('edit-langcode', 'fr');
    +    $this->assertOption('edit-langcode', LanguageInterface::LANGCODE_NOT_SPECIFIED);
    +    $this->assertOption('edit-langcode', LanguageInterface::LANGCODE_NOT_APPLICABLE);
    +    $this->clickLink(t('Add redirect'));
    

    Some of those options here don't make sense. This should only list:

    * All
    * unspecified (maybe labeled as "Any language")
    * the actual langauges..

    for implementation, it might be easiest to get the real languages and then just preprend the two others.

15. +++ b/modules/redirect_404/src/Tests/Fix404RedirectUILanguageTest.php
    @@ -0,0 +1,222 @@
    +    // Check the error path visit count.
    +    $xpath = '/html/body/div/main/div/div/div/div/div[2]/table/tbody/tr/td[2]';
    

    Same here. There's only one one table, //table/tbody/tr/td is specific enough.

16. +++ b/modules/redirect_404/src/Tests/Fix404RedirectUILanguageTest.php
    @@ -0,0 +1,222 @@
    +    $this->drupalGet('admin/config/search/redirect/404');
    +    $this->assertTrue(strpos($this->xpath($body)[0]->asXML(), 'French'));
    +    $this->assertTrue(strpos($this->xpath($body)[0]->asXML(), 'English'));
    

    Same for those additional language checks. why use this for those but assertText() for everything else?

17. +++ b/modules/redirect_404/tests/src/Kernel/Fix404RedirectCronJobTest.php
    @@ -0,0 +1,137 @@
    +    // Add some records in the redirect_404 table for test.
    +    for ($i = 0; $i < 10; $i++) {
    +      db_query("INSERT INTO {redirect_404} (path, langcode, count, timestamp, resolved, relevancy) VALUES (:path, :langcode, 1, 1465082407, 0, 1.00)", [
    +        ':path' => '/test' . $i,
    +        ':langcode' => 'en'
    +      ]);
    +    }
    +
    +    // Check that there are 10 rows in the redirect_404 table.
    +    $result = db_query("SELECT COUNT(*) as rows FROM {redirect_404}")->fetchField();
    +    $this->assertEquals(10, $result);
    +
    +    // Update/hardcode the relevancy for each row to not delete all rows after
    +    // running the cronjob.
    +    db_update('redirect_404')->fields(['relevancy' => '91.97781'])->condition('path', '/test5')->execute();
    +    db_update('redirect_404')->fields(['relevancy' => '82.63037'])->condition('path', '/test6')->execute();
    +    db_update('redirect_404')->fields(['relevancy' => '71.79651'])->condition('path', '/test2')->execute();
    +    db_update('redirect_404')->fields(['relevancy' => '67.85126'])->condition('path', '/test7')->execute();
    +    db_update('redirect_404')->fields(['relevancy' => '53.98305'])->condition('path', '/test4')->execute();
    +    db_update('redirect_404')->fields(['relevancy' => '42.81542'])->condition('path', '/test3')->execute();
    +    // The following rows have smaller relevancy than the cutoff value defined
    +    // by the row_limit and will be dropped.
    +    db_update('redirect_404')->fields(['relevancy' => '37.99983'])->condition('path', '/test9')->execute();
    +    db_update('redirect_404')->fields(['relevancy' => '28.99964'])->condition('path', '/test1')->execute();
    +    db_update('redirect_404')->fields(['relevancy' => '15.79511'])->condition('path', '/test0')->execute();
    +    db_update('redirect_404')->fields(['relevancy' => '3.929704'])->condition('path', '/test8')->execute();
    +    redirect_404_cron();
    

    Getting there :)

    Why dont you just do 10 db_insert() queries in the first place instead of a loop and then update them?

Done as suggested. Points 1, 6 need discussion, 13, 14, 16, 17 still need work, i ll do it tomorrow.

Fixed and cleaned code in the Fix404RedirectUITest and Fix404RedirectUILanguageTest, created Redirect404TestBase AssertRedirectTrait which are imported in the 404 webtests, and cleaned Fix404RedirectCronJobTest kerneltest as suggested.

Still have points 1 and 6 left.

+++ b/modules/redirect_404/src/Tests/Redirect404TestBase.php
@@ -0,0 +1,143 @@
+    'dblog',

dblog should no longer be required now.

Otherwise I think we're done, with the exception of the last two points there that we need to discuss with @miro_dietiker.

Right, done.

Fixed last points (1 and 6), changed that docblock in the RedirectFix404Form and moved the cron settings into the redirect settings.
Sorted! :D

+++ b/modules/redirect_404/redirect_404.module
@@ -20,25 +21,28 @@
- * Implements hook_form_FORM_ID_alter() for system_logging_settings().
+ * Implements hook_form_alter().
  */
-function redirect_404_form_system_logging_settings_alter(&$form, FormStateInterface $form_state) {
-  $row_limits = [100, 1000, 10000, 100000, 1000000];
-  $form['redirect_404_row_limit'] = [
-    '#type' => 'select',
-    '#title' => t('404 error database logs to keep'),
-    '#default_value' => \Drupal::configFactory()->getEditable('redirect_404.settings')->get('row_limit'),
-    '#options' => [0 => t('All')] + array_combine($row_limits, $row_limits),
-    '#description' => t('The maximum number of 404 error logs to keep in the database log. Requires a <a href=":cron">cron maintenance task</a>.', [':cron' => \Drupal::url('system.status')])
-  ];
+function redirect_404_form_alter(&$form, FormStateInterface $form_state, $form_id) {
+  if ($form_id == 'redirect_settings_form') {
+    $row_limits = [100, 1000, 10000, 100000, 1000000];

too much change, you can implement this just like before, with a FORM_ID based alter. you just need to do a search replace from one form id to the other.

Sorted :P

Now for real.. ^^'

Ok, I'm out of things to complain about.

I think @larowlan plans to review/test this soon as well.

One issue and some general observations

1. +++ b/modules/redirect_404/redirect_404.routing.yml
   @@ -0,0 +1,7 @@
   +redirect.fix_404:
   

   Redirect module still has a menu local task pointing to this, it need to be moved too. See redirect.links.task.yml

2. +++ b/modules/redirect_404/redirect_404.views.inc
   @@ -0,0 +1,99 @@
   +  if (!\Drupal::service('redirect.not_found_storage') instanceof SqlRedirectNotFoundStorage) {
   

   Should this typehint a new interface instead of the concrete implementation?

3. +++ b/modules/redirect_404/src/Form/RedirectFix404Form.php
   @@ -0,0 +1,132 @@
   +    $languages = \Drupal::languageManager()->getLanguages(LanguageInterface::STATE_ALL);
   +    $multilingual = \Drupal::languageManager()->isMultilingual();
   ...
   +    $redirect_storage = \Drupal::service('redirect.not_found_storage');
   ...
   +      $row['timestamp'] = \Drupal::service('date.formatter')->format($result->timestamp, 'short');
   ...
   +      if (\Drupal::entityTypeManager()->getAccessControlHandler('redirect')->createAccess()) {
   
   +++ b/modules/redirect_404/src/Plugin/views/field/Language.php
   @@ -0,0 +1,24 @@
   +    return \Drupal::languageManager()->isMultilingual();
   

   nit:These could all be injected

4. +++ b/modules/redirect_404/src/SqlRedirectNotFoundStorage.php
   @@ -0,0 +1,120 @@
   +    $row_limit = \Drupal::config('redirect_404.settings')->get('row_limit');
   

   There's a hidden dependency here that could be injected too

Yay, awesome. :-)

Just for sake of completeness, potential follow-ups discussed are:
- Implement a redis backend, because it seems to be a great fit and is popular in D8 enterprise environments, at least for us.
- Add an "ignore" action on a 404 record that makes it hide permanently, even if repeating again.
- Figure out how we can get rid off the watchdog 404 entries.