Create a redirect for the new Block types path

We discussed this problem on #2862564: Move Custom block library to Content, which is related to #2987964: Move custom block types admin link to admin/structure. We agreed to postpone #2862564 on this issue, and we may revert #2987964 if we cannot fix this issue before Drupal 10.1.0-alpha1. See Comments #120, #121, #124, #127, #130, #131 on Issue #2862564. I added both as related issues.

We suggested entity.block_content.collection.bc as the new route name.

We suggested escalating the warning message to an error in Drupal 11 and removing the route in Drupal 12. I added #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name as a related issue: if that issue is fixed, then we should follow the procedure there for deprecating a route before removing it.

I think this issue is properly a bug report, not a task.

MR !3244 implements the proposed resolution.

I included a test for the deprecation message. Should I also include a test for the warning message?

I forgot to unassign myself when I set the status to NR.

I am queueing a re-test.

I am adding #3333394: [policy, no patch] Determine a best practice for providing BC for internal paths that change between minor releases as a related issue. We can document there what we decide to do on this issue.

The static analysis tools insist on a specific version in the deprecation message, and I targeted drupal:12.0.0. That is based on my suggestion from #2862564-124: Move Custom block library to Content:

A log message might be more helpful: a warning in Drupal 10, an error in Drupal 11, and then remove the BC route in Drupal 12.

Maybe that is overly complicated. A 404 response (page not found) is logged as a warning, so it is odd to issue an error for the redirect. Perhaps we can agree to delete the route in Drupal 11, without first deprecating it (process for that to be decided in #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name). Are these steps enough to delete a route without first deprecating it?

1. The route name ends in ".bc", indicating that it is for backwards compatibility.
2. When the route is added, there is a code comment in the .routing.yml file saying when it will be removed.
3. The controller for the route triggers a deprecation message.

The current MR already logs warnings, so back to NR:

    $this->getLogger('block_content')->warning('The page /admin/structure/block/block-content/types has been moved to /admin/structure/block-content. Update links and shortcuts.');

I think the remaining question is related: when do we remove the BC route, in D11 or D12? My original suggestion was to escalate the warning to an error in D11 and then remove the route in D12. I am having second thoughts: see Comment #7.

There is also room for some wordsmithing.

I think we have consensus on removing the BC route in Drupal 11, even if we do not deprecate it first. Updating the version in the deprecation messages will be pretty easy.

In fact, I just did it. Now I do not have to apologize for leaving this issue at NR.

Do we have consensus on whether to use the messenger service?

@smustgrave: Thanks for adding the message. I was planning to do that after I saw #22, but you got to it first.

We will remove the BC route before Drupal 11, not as part of #3333394: [policy, no patch] Determine a best practice for providing BC for internal paths that change between minor releases, so I updated the comment in the .routing.yml file. I made the comment a deprecation notice and a reference to the CR.

My thinking is that I want to make it easy to remove the BC route when we are preparing for D11. I assume that we do that by searching for deprecation notices. The MR already adds a deprecation notice to the controller, and we might know to remove the route when we remove the controller. I think it is more reliable to add a separate deprecation comment.

I reviewed the comments and patch from #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name, and I searched for "deprecat" on the Symfony docs https://symfony.com/doc/current/routing.html. I considered adding a deprecated: section to the BC route, but it looks as though that is specifically intended for route aliases (changing the route name) so I decided not to.

I am updating the issue summary now, and I will update the change record next.

I took @xjm's suggestions for the comment in the routing file and for the message text, except

• In the comment, I @see the change record for this issue instead of #3333394: [policy, no patch] Determine a best practice for providing BC for internal paths that change between minor releases.
• In the message, I deleted "automatically" because "less is more".

I am setting the status to NW to review the message text and update the screenshot. I will add the current message text to the issue summary.

I am updating the screenshot in the issue description. My local site is running 10.1.x with the Umami demo profile (Claro theme).

While reviewing the message text, we should consider that the title of the page also changed. When it was a sub-tab on the "Custom block library" page, the page title was "Custom block library" and the sub-tab title was "Block types". Now there are no tabs (local tasks) involved, and the page title is "Custom block types".

Usability review

We discussed this issue at #3335028: Drupal Usability Meeting 2023-01-27. That issue will have a link to a recording of the meeting.

For the record, the attendees at today's usability meeting were @BlackBamboo, @benjifisher, @ckrina, @iszabo, @shaal, and @simohell.

We agreed that there are some problems with the current text:

1. It is too long.
2. The page title (Custom block types) is not accurate. Before Drupal 10.1, the page title is "Custom block types page" and the sub-tab (local task) is labeled "Block types".
3. The path may not be accurate: on a multilingual site, it should include the language prefix.

In order to keep the message short, we agreed that it is not important to explain that the redirect will be removed in a later version. We suggest the following:

You have been redirected from /admin/structure/block/block-content/types. Update links, shortcuts, and bookmarks to use the current page: /admin/structure/block-content.

We could make it even shorter: leave off the current path and let the user get it from the browser. Or provide a link:

You have been redirected from /admin/structure/block/block-content/types. Update links, shortcuts, and bookmarks to use the current page.

A full URL will be more useful for external bookmarks, and providing the link will help if the browser hides the current URL. This message explains the situation and how to fix it. The only thing missing, for brevity, is the urgency (fix it before D11).

We should be able to get the legacy path (with language code, if the site is multilingual) from the BC route. We should update the MR and the issue description (text and screenshot).

I am addressing @AaronMcHale's comment from the MR.

I am also updating the issue summary and attaching new screenshots:

Standard profile (one language):

You have been redirected from /admin/structure/block/block-content/types. Update links, shortcuts, and bookmarks to use the current page.

Umami demo profile (multilingual):

You have been redirected from /en/admin/structure/block/block-content/types. Update links, shortcuts, and bookmarks to use the current page.

Finally, I am logging a different message from the one shown to the user. The log message includes the note about removing the redirect, and it refers to the change record. I am pretty sure that log messages are not supposed to be translated.

The test passes locally:

benji@drupal-web:/var/www/html$ phpunit -c core core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
PHPUnit 9.5.26 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\block_content\Functional\BlockContentTypeTest
......                                                              6 / 6 (100%)

Time: 00:15.732, Memory: 4.00 MB

OK (6 tests, 140 assertions)

HTML output was generated
...

I queued a retest, but I am not optimistic.

I think the test should use an explicit path:

    $base_path = parse_url($this->baseUrl, PHP_URL_PATH) ?? '';
    $this->assertSession()->pageTextContains("You have been redirected from $base_path/admin/structure/block/block-content/types. Update links, shortcuts, and bookmarks to use this page.");

Since @smustgrave and I have both worked on the MR, neither of us should set the issue status to RTBC. I am setting it back to NR. Besides, we have to come to an agreement on "this page" vs. "the current page".

We looked at this issue in #3335028: Drupal Usability Meeting 2023-01-27. (See Comment #34.) I am removing the tag for a usability review.

There is still one remaining task from the issue description:

1. Document our decisions on #3333394: [policy, no patch] Determine a best practice for providing BC for internal paths that change between minor releases.

That documentation should include what to do for less restricted pages (Comment #22):

We should use the messenger service and display a warning to the user upon redirect, yes. The answer would be different if the path did not currently require a restricted access permission, though. If it were available to content authors or especially end users, we would use the three-major approach of log+deprecate in a minor, warn in a major, and remove in the following major.