Create BC redirects for children of changed paths

I am adding one item to the proposed resolution: update existing redirects to use the new trait.

I considered postponing this issue on #3318112: [PP1] Move "Block layout" from Structure to Appearance, but it would not be hard to copy the helper function from the MR on that issue, so I think the two issues can be done in either order.

I am adding the issues mentioned in the issue summary as related issues.

How should we handle the destination query parameter?

1. Leave it alone.
2. Fix it (replacing old path with new path).
3. Remove it.

I do not think that (2) is viable. For example, the Configure link (before #3318112: [PP1] Move "Block layout" from Structure to Appearance, using the Umami profile) is /en/admin/structure/block/manage/claro_page_title?destination=/en/admin/structure/block/list/claro. The "old path" is /admin/structure/block/manage/{plugin_id} and we need to update "structure" to "appearance".

If we use (2), then we have to hope that we created a redirect for the destination, if it is also updated. Assuming we did, then there will be another warning in the admin UI.

I vote for (3).

I started thinking about the single-responsibility principle, and I realized that the helper function in #3318112: [PP1] Move "Block layout" from Structure to Appearance is an abomination. Instead of a trait, we should have a helper class and use it in a controller like this:

  $change_record = 'https://www.drupal.org/node/3320855';
  $helper = new PathChangedHelper($route_match, $request, $change_record);
  $params = $helper->messageParameters();
  $this->logger->warning('A user was redirected from %old_path. This redirect will be removed in a future version of Drupal. Update links, shortcuts, and bookmarks to use %new_path. See %change_record for more information.', $params);
  $this->messenger->addWarning('You have been redirected from %old_path. Update links, shortcuts, and bookmarks to use %new_path.', $params));
  return $helper->redirect();

I have a first draft of the helper function in the MR, so I am setting the issue status to NW. I am updating the first part of the Proposed resolution in the issue summary.

I think it does not make sense to pass the URL for the change record to the helper function only to have it sent back as one of the parameters. It is also not a great idea for the helper function to decide the placeholder names. So the new version is used like this:

  $helper = new PathChangedHelper($route_match, $request);
  $params = [
    '%old_path' => $helper->oldPath(),
    '%new_path' => $helper->newPath(),
    '%change_record' => 'https://www.drupal.org/node/3320855',
   ];
  $this->logger->warning('A user was redirected from %old_path. This redirect will be removed in a future version of Drupal. Update links, shortcuts, and bookmarks to use %new_path. See %change_record for more information.', $params);
  $this->messenger->addWarning('You have been redirected from %old_path. Update links, shortcuts, and bookmarks to use %new_path.', $params));
  return $helper->redirect();

I wonder whether, for consistency, I should replace the redirect() method with one that just returns the full URL as a string.

I made the changes suggested on the MR and implemented Step 2 in the proposed resolution.

The new function bodies are the same number of lines as the old ones, only slightly simpler. The doc blocks and parameter lists are more complicated, since they have to declare the route match and request objects. At this point, it is not clear that the helper class (and the extra level of indirection) is worth it. But the existing controllers are as simple as possible: no arguments, no query parameters. In #3318112: [PP1] Move "Block layout" from Structure to Appearance, we have to deal with both, and I think it is worth handling arguments and query parameters (and stripping destination) in one place.

One change is that the new code uses Url::getInternalPath() instead of Url::toString(). That means the warning messages will show paths like admin/content/block-content. The old message included at least a leading /; a language prefix like /en on Umami; a directory if Drupal is installed in a subdirectory. Is this a bug or a feature?

The issue status is still NW for the last step in the proposed resolution and for tests.

@AaronMcHale:

Have you looked at the helper function in #3318112: [PP1] Move "Block layout" from Structure to Appearance? It does pretty much what you suggest.

I think the individual controller has to be responsible for the messages: deprecation, log, user. We may need a warning or an info or none, depending on which path is being changed and the permissions typically required for that path. I think that sort of variation was discussed on #2862564: Move Custom block library to Content. Managing that variation in the helper function is clunky. As I said in #7 on this issue, I used the single-responsibility principle as a guide.

Once the messages are in the individual controller, I think it makes sense to let that function decide what placeholders to use. But if we want to save a few lines of boilerplate, then we could go back to providing a method messageParameters() as in Comment #7.

Follow-up to my comment in #9: even though it is the same number of lines, the advantage of the current version is that it does not hard-code the route name. That means the same controller can be used by different routes. This will be important when we get to the last step in the proposed resolution.

@AaronMcHale:

I am glad we agree!

I rebased on the current 10.1.x, and added three commits (tiny, small, large).

Adding redirect routes for all the child paths was more work than I expected. I am sure there is room for improvement. It all makes sense to me now, but if it could use some code comments, I am happy to cooperate.

I am setting the status to NR. On the theory that no good deed goes unpunished, I am sure that someone will point out that it needs tests.

I am removing this question from the issue summary:

To be decided: should the new routes be defined in the block_content module, so that all the related BC routes are in the same place? Or should the routes be defined in the modules that create the original routes, as a model for contributed modules to follow?

The routes for the child paths are defined in a new route subscriber for the block_content module. This is a model that other modules can follow.

I may not have time to update the MR today, but after sleeping on it, I think there is a much more reliable way to generate the routes that need BC variants. Here is a little test script:

use Drupal\Core\Routing\RouteBuildEvent;
use Drupal\field_ui\Routing\RouteSubscriber;
use Symfony\Component\Routing\RouteCollection;

$router = \Drupal::service('router');
$collection = $router->getRouteCollection();
$test_collection = new RouteCollection();
$base_route = $collection->get('entity.block_content_type.edit_form');
$test_collection->add('entity.block_content_type.edit_form', $base_route );
foreach ($test_collection as $r) { echo $r->getPath() . PHP_EOL; }
$event = new RouteBuildEvent($test_collection);
$etm = Drupal::entityTypeManager();
$subscriber = new RouteSubscriber($etm);
$subscriber->onAlterRoutes($event);
foreach ($test_collection as $r) { echo $r->getPath() . PHP_EOL; }

I removed the use statement flagged in Comment #15 and resolved merge conflicts after #3318549: Rename Custom Block terminology in the admin UI. Back to NR. (But I still plan to make the change I suggested in #14.)

@smustgrave:

Thanks for the review, but I think it is premature to call this issue RTBC. There have been changes since @larowlan last commented on the MR, and I just now implemented the idea from #14. (It ends up being a few lines longer, but I still think it is better than hard-coding the list as in the earlier versions.)

I am setting the status back to NR. To be honest, it should probably be NW for tests.

I made one change to the config_translation module. I will add a note to the proposed resolution explaining that.

This should fix at least some of the failing tests. It is the config_translation module again.

I still blame the config_translation module. Instead of fighting it, I am going back to an explicit list of routes provided by that module and reverting the changes I made to that module.

I rebased the MR on the current 10.1.x and resolved the merge conflict. (It was a simple add/add conflict.)

I also added two unit tests and a kernel test for the helper class. The second unit test and the kernel test are redundant, and I would like a second opinion on which one to keep. I think that the unit test is too brittle, so it is worth the extra 510 ms to run the kernel test.

Back to NR.

I thought I tested when I made fc02d5d553c268b52cff2586852bb83304107100, but I must have been too tired. I just reverted that commit, and now the test passes locally.

@smustgrave:

Thanks for testing.

Did you consider the question about tests in #24? Do we want to keep the second unit test or the kernel test?

I updated the change record. While I was doing that, I noticed that one of the redirect controllers referred to #2317981: Move block content edit and delete routes under admin/content/block to improve IA for editors and fix breadcrumbs instead of the change record, so I updated the controller and the related test.

I removed the second unit test. Based on #31, I am setting the status to RTBC.

When I created this issue, I thought that #3318112: [PP1] Move "Block layout" from Structure to Appearance would be fixed first. That seems unlikely now, so I am rewriting the proposed resolution: this issue adds a new helper class, but does not replace what would have been added by that issue.

@Rassoni:

Thanks for fixing the CodeSniffer issues. Back again to RTBC, based on #31. That should trigger a retest.

The failing test is Drupal\BuildTests\Composer\Component\ComponentsIsolatedBuildTest. It does not seem related, and it passes locally, so let's keep the status at RTBC.

@larowlan:

I am glad to see this get in!

Can you add a link to the follow-up issue you created?

I think we need to include $block_content in the parameter list, with its type hint, so that we can have the same access checks on the BC route as on the original. In other words, it is used implicitly.

OTOH, maybe we should not do access checks at all on the BC route. (shrug)

The doc block now says "an deprecated". Oh, well.