Fix the migration of the status_code redirect property: status code of NULL or 0 causes exception

Curious if you found a solution here. I had the same condition. I ended up going into the database directly and running a bit of sql to add the status code. Seemed to work.

Hey, I have the same error in my log and my Website was also created with the D7 migration module.
@maddentim can you share with us your SQL script ?

I have same problem too, after update from D7 to D8

I've also come across this issue and it seems the root problem is that D8 redirects do not behave in the same ways as D7. In D7 if the individual redirect is not set then it uses the system-wide default. But in D8 this default is only used when new redirects are created. So one approach would be to fix this behaviour.

Or it may be possible to enhance the migration to use the default if the response code is not set for a redirect.

Or if you want the quick fix in SQL, then first check what the current status is with:
SELECT count(*), status_code FROM redirect group by status_code;
In my case a few were already set to 301, but most were NULL. So I was OK to just set everything with:
update redirect set status_code=301;
Boom! (drush cr) Done!
To be a little more selective, safer and performant then:
update redirect set status_code=301 where status_code is NULL;

I got into this problem too in the context of a d7 to d8 migration.
@rd.michael evaluation of the reason is the same I found for my case: d7 redirect_default_status_code can be unset.

I am attaching several patches here.

• 3082364-4: the original change by @rd.michael, thanks!
• 3082364-7-tests-only: adds a test with the failing case, should fail.
• 3082364-7: A modified version of 3082364-4 including a test, and some re-organization of the existing one to help code reuse.
• interdiff-3082364-4-7: Changes from 3082364-4 to 3082364-7.

The minimal patch for the fix would be the following.

diff --git a/src/Plugin/migrate/source/d7/PathRedirect.php b/src/Plugin/migrate/source/d7/PathRedirect.php
index c9ec720..bee41b2 100644
--- a/src/Plugin/migrate/source/d7/PathRedirect.php
+++ b/src/Plugin/migrate/source/d7/PathRedirect.php
@@ -42,6 +42,9 @@ class PathRedirect extends DrupalSqlBase {
         ->condition('name', 'redirect_default_status_code')
         ->execute()
         ->fetchField());
+      // Provide the default value used on d7 variable_get() calls when the
+      // relevant variable is not set.
+      $default_status_code = $default_status_code ?: 301;
     }
     $current_status_code = $row->getSourceProperty('status_code');
     $status_code = $current_status_code != 0 ? $current_status_code : $default_status_code;

I have not tried the tests locally yet, so let us see what testbot says.
Also, I am not sure if we want to reuse code from fixtures, and if so, how; feedback is welcomed.

I am working with @marvil07 on a migration project, and I tested his patch from #7 on that project. It works as expected.

Thanks for adding a test, and for including a test-only patch! I did not review the test very thoroughly, but I did check that the test-only patch fails in the expected way and that it is, in fact, a test-only version of the main patch. (The interdiff utility gets confused, but a direct diff of the two patches is easy.)

Outside the test, the change is simple and, as @marvil07 said, minimal. I am changing the issue status to RTBC.

After the patch, I see the following code in the source plugin:

  public function prepareRow(Row $row) {
    static $default_status_code;
    if (!isset($default_status_code)) {
      $default_status_code = unserialize($this->getDatabase()
        ->select('variable', 'v')
        ->fields('v', ['value'])
        ->condition('name', 'redirect_default_status_code')
        ->execute()
        ->fetchField());
      // Provide the default value used on d7 variable_get() calls when the
      // relevant variable is not set.
      $default_status_code = $default_status_code ?: 301;
    }
    $current_status_code = $row->getSourceProperty('status_code');
    $status_code = $current_status_code != 0 ? $current_status_code : $default_status_code;
    $row->setSourceProperty('status_code', $status_code);
    return parent::prepareRow($row);
  }

I would rather use a class property, set in the constructor, than a static variable in a class method. I would also simplify the last two lines before the return to

    $row->setSourceProperty('status_code', $current_status_code ?: $default_status_code);

(untested). Both of these points are out of scope for fixing the bug, but I will be happy to review an updated patch if anyone wants to make the changes.

Can't apply patch to stable 8.x-1.6 D8.8.

@M@ster:

The latest tag (8.x-1.6) is the same as the HEAD of the 8.x-1.x development branch. The patch in #7 applies cleanly when I try it.

How are you applying the patch?

Simplified the default value assignment (DrupalSqlBase provides method for these kind of cases) and changed the value comparison to a clearer one.

Review of #14

1. +++ b/src/Plugin/migrate/source/d7/PathRedirect.php
   @@ -36,18 +36,14 @@ class PathRedirect extends DrupalSqlBase {
   -      $default_status_code = unserialize($this->getDatabase()
   -        ->select('variable', 'v')
   -        ->fields('v', ['value'])
   -        ->condition('name', 'redirect_default_status_code')
   -        ->execute()
   -        ->fetchField());
   ...
   +      $default_status_code = $this->variableGet('redirect_default_status_code', 301);
   

   👍 This was indeed duplicating the logic of \Drupal\migrate_drupal\Plugin\migrate\source\DrupalSqlBase::variableGet(), so this looks like a solid improvement to me!

2. +++ b/src/Plugin/migrate/source/d7/PathRedirect.php
   @@ -36,18 +36,14 @@ class PathRedirect extends DrupalSqlBase {
   -    $status_code = $current_status_code != 0 ? $current_status_code : $default_status_code;
   +    $status_code = !empty($current_status_code) ? $current_status_code : $default_status_code;
   

   👍 This change makes sure we do not rely on an explicitly non-strict comparison, which is a crucial yet easy to miss detail. Therefore this code seems more futureproof.

Review of overall patch

1. +++ b/tests/src/Kernel/Migrate/d7/PathRedirectTestBase.php
   @@ -0,0 +1,49 @@
   +    /** @var Redirect $redirect */
   

   🤓 This should use the FQCN.

2. +++ b/tests/src/Kernel/Migrate/d7/PathRedirectTestBase.php
   @@ -0,0 +1,49 @@
   +    $this->assertSame($this->getMigration('d7_path_redirect')
   +      ->getIdMap()
   +      ->lookupDestinationIds([$id]), [[$redirect->id()]]);
   +    $this->assertSame($source_url, $redirect->getSourceUrl());
   +    $this->assertSame($redirect_url, $redirect->getRedirectUrl()
   +      ->toUriString());
   +    $this->assertSame($status_code, $redirect->getStatusCode());
   

   🤓 The formatting here is really weird. But … pre-existing. So … 🤷‍♂️

3. +++ b/tests/src/Kernel/Migrate/d7/PathRedirectWithoutDefaultTest.php
   @@ -0,0 +1,35 @@
   +  public function testPathRedirect() {
   +    $this->assertEntity(5, '/test/source/url', 'base:test/redirect/url', '301');
   +    $this->assertEntity(7, '/test/source/url2', 'http://test/external/redirect/url?foo=bar&biz=buz#fragment-1', '301');
   +  }
   

   ✅ I was going to say: why not use a @dataProvider? But that'd be too slow; migrations are very slow.

I was not able to apply patch #14 using composer using redirect 1.6

I applied patch #14 to 1.6, but didn't work. So I followed #6 and it worked.

That's odd, because we're applying #16 on top of drupal/redirect version 1.6 just fine 🤔

Follow-up to #16.2:

-    $status_code = $current_status_code != 0 ? $current_status_code : $default_status_code;
+    $status_code = !empty($current_status_code) ? $current_status_code : $default_status_code;

Why not

$status_code = $current_status_code ?: $default_status_code;

instead? Or even

$status_code = $row->getSourceProperty('status_code') ?: $default_status_code;

Or see my suggestion at the end of #10.

This bug was found and a fix posted a year ago.

Critical bug (site is broken, PHP exception).
Fix is obvious (clear what's wrong).
Fix is trivial (simple one liner).
Tested by community.

Enough bike shedding and optimizing the leap year interrupt. I've wasted yet-another morning on an already-fixed-but-not-committed bug.

Committed, thanks.