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 📌 [PP1] Move "Block layout" from Structure to Appearance Postponed , 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 📌 [PP1] Move "Block layout" from Structure to Appearance Postponed , 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).

Yeah 3 sounds best

I started thinking about the single-responsibility principle, and I realized that the helper function in 📌 [PP1] Move "Block layout" from Structure to Appearance Postponed 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 📌 [PP1] Move "Block layout" from Structure to Appearance Postponed , 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.

Grat work on this, thanks Benji!

I wonder if it's worth bringing the log and warning messages into this helper class. My thinking is that it will help us enforce uniformity and reduce code duplication across core. This is effectively a new pattern that we've established for the admin UI and I'd love to (eventually, when I have time) document this in the user interface standards (or somewhere similar).

So thinking like that, to me it makes sense to bring as much of the "boilerplate" code that we can into the helper class to help enforce a uniform pattern across routes that use this.

Maybe even going as far as provide a standard controller class that BC routes can simply point to, rather than each route having to create basically the same controller over and over again.

@AaronMcHale:

Have you looked at the helper function in 📌 [PP1] Move "Block layout" from Structure to Appearance Postponed ? 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 ✨ Move Custom block library to Content Fixed . 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.

@benjifisher

On reflection I agree with you that the messages would vary a lot between implementations so it's probably not worth trying to abstract them away, as you said it could get clunky.

@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; }

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Seems @larowlan has already done a review and all threads have been addressed.

Do wonder if it deserves it's own change record for how contrib can leverage.

@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.

Seems the new changes did cause some failures in the tests.

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

Progress! Still some failures but think it's closer.

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.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

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.

Can't seem to rerun the tests. Can you take a look at that failure?

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.

All threads appear to still be answered.

Applied patch locally and tested some paths /admin/structure/block/block-content and /admin/structure/block/block-content/types and I still get the warning message as expected.

Failure appears to be random ckeditor5 and unrelated to this issue.

Can we update the change record to include the changes here?

Then think good to mark RTBC.

@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 📌 Move block content edit and delete routes under admin/content/block to improve IA for editors and fix breadcrumbs Fixed instead of the change record, so I updated the controller and the related test.

Personally for me I'm more a fan of kernel tests. So if we are voting that would be mine.

Just to remove 2nd unit test per #28. Can RTBC after that.

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

When I created this issue, I thought that 📌 [PP1] Move "Block layout" from Structure to Appearance Postponed 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.

Looks like the MR is failing with PHPCS checks

Rassoni → made their first commit to this issue’s fork.

@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.

Left some comments on the MR, I think we can push the 'auto detect child paths' idea to a follow up and get this in.

Fine to self RTBC

On second thoughts, just going to get this in by making the change myself at commit time

Committed to 11.x and backported to 10.1.x

Fixed on commit:

diff --git a/core/lib/Drupal/Core/Routing/PathChangedHelper.php b/core/lib/Drupal/Core/Routing/PathChangedHelper.php
index 650c110f27f..e8fb996680c 100644
--- a/core/lib/Drupal/Core/Routing/PathChangedHelper.php
+++ b/core/lib/Drupal/Core/Routing/PathChangedHelper.php
@@ -14,7 +14,7 @@
  * compatibility (BC). If the original route is example.route, then the BC route
  * should be named example.route.bc.
  *
- * The controller for the BC route should have an @deprecated annotation, a
+ * The controller for the BC route should have an deprecated annotation, a
  * deprecation error, and type declarations for any parameters that are required
  * for access checking. Then the body of the controller can use the methods
  * provided by this class:

Opened 📌 Attempt to auto-detect child paths of deprecated routes and add redirects Active for follow-up

larowlan → closed merge request !3801

@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.

Follow-up is in #42 📌 Attempt to auto-detect child paths of deprecated routes and add redirects Active

I'll hotfix the deprecated, I assume the `an` was for reading it as `an at deprecated` and now that makes no sense without the `at`

I hotfixed the an > a

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