system_site migrates default 403, 404 and front page paths as forward slash (/)

Nice find, @huzooka

I reproduced the bug using the test fixture and migrate_drupal_ui. Before migrating, I modified the 403 setting on the D7 site.

The config gets migrated incorrectly, as described in the IS and shown in the screenshot below.

And indeed, a 403 redirects a user to the front page.

After applying the patch and redoing the migration into a fresh D9 install, the bug is fixed as shown below.

And the user is indeed directed to the default Access Denied page on 403.

The update to the migration yml looks great. And the test coverage looks appropriate.

But the inclusion of the drupal_weight_select_max in this patch is odd to me. If we want to include that fix in this issue, I think we need to add test coverage for that as well. The new test coverage is only for the 403/404 stuff.

So I'm setting this back to NW to either

1. remove the changes related to drupal_weight_select_max (and perhaps put that in a new issue)

or

2. add test coverage related to drupal_weight_select_max

Not a full review here, but I want to respond to a comment in #5:

This approach was wrong before: I assumed that we will have the default configuration values on the destination Drupal instance. But we shouldn't assume this! We have to migrate the appropriate config value from the source.

Absolutely correct. I missed this when I was reviewing the patch in #2. Assume the config on the source site looks like this:

If the config on the destination site looks like this prior to migration:

Then with the patch in #2, the destination looks like this after migration:

Which is wrong because we want Default 403 to be empty. Skipping the process prevents us from overwriting a non-default value on the destination site.

I wanted to post this while it was fresh because it's kind of tricky. Hopefully I'll be able to review and test more fully later.

I applied the patch in #10 and basically repeated what I did in #3 and #12. The 403 and 404 settings appear to get migrated correctly now.

But there's a bug with the site_frontpage migration. The fixture has /node as the frontpage which means the destination site ends up with '' as the frontpage, which results in this when viewing the Site Information Form:

InvalidArgumentException: Source path has to start with a slash. in Drupal\path_alias\AliasManager->getAliasByPath() (line 185 of core/modules/path_alias/src/AliasManager.php).

The problem is that even though the default frontpage looks like an empty string, under the hood it is REALLY /user/login.

So the static_map needs to be updated for the frontpage migration:

map:
        # Drupal 6 and Drupal 7 default site_frontpage is 'node'. If this
        # variable is set to 'node', to an empty string, or it is completely
        # missing, we want to migrate the equivalent Drupal 9 value, which is
        # /user/login.
        '/node': '/user/login'
        '/': '/user/login'

And this has me wondering if the 403/404 static maps should be updated to map /node to '/'?

'page/403':
    -
      plugin: concat
      source:
        - constants/slash
        - site_403
    -
      plugin: static_map
      map:
        '/': ''
        '/node': '/' #Should we add this mapping?
      bypass: true

I'm not personally as familiar with D6 and D7. But it seems like if the frontpage migration understands '/node' to mean 'default frontpage', then the 403/404 migrations should probably treat '/node' to mean 'default frontpage'. I'm not totally sure though.

Note that the default value for 403 and 404 in D9 really is an empty string. It's ok to map '/' to ''.

Back to NW to fix the static map for the frontpage. I'd also like to hear your thoughts on whether /node should get mapped in 403/404.

Adding a related issue: #2607754: Add intelligence to front page configuration migration

There is detail in these comments about the result of the migrations which I think make the comment just that bit longer and harder to get. And beginning with an 'if' is odd when these are a simple deletion.

@huzooka and @danflanagan8, thanks for working on this!

Just a few suggestions to comments.

1. +++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateSystemConfigurationTest.php
   @@ -133,6 +133,22 @@ protected function setUp(): void {
   +    // If site_403 or site_404 aren't set or they are set to an empty string,
   +    // we have to migrate empty value.
   

   Just a suggestion, "Delete 'site_403' in order to test the migration of a non-existing error page link."

2. +++ b/core/modules/system/tests/src/Kernel/Migrate/d7/MigrateSystemConfigurationTest.php
   @@ -133,6 +133,22 @@ protected function setUp(): void {
   +    // If drupal_weight_select_max is missing, then its default value is 100
   +    // (See DRUPAL_WEIGHT_SELECT_MAX in system.module), and we should
   +    // overwrite the preexisting config value.
   

   Just a suggestion, "Delete 'drupal_weight_select_max ' in order to test the migration when it is not set."

It's nice when things turn out to be simpler than more complex for once 😊

This looks great to me!

This simplifies our situation here because we can map the empty front page variable to /node.

I'm not 100% convinced this is right. Isn't the D9 Views frontpage view that provides the response for /node? That's a problem if Views is not enabled on the destination site, as described in the related issue #2607754: Add intelligence to front page configuration migration.

But maybe the best thing to do is go with the patch in #16 for this issue, knowing that the situation described in the related issue might arise.

Committed and pushed 914fadac64 to 10.0.x and 310943d16a to 9.4.x and 79ceee0c75 to 9.3.x. Thanks!