Create a redirect for the new Block types path

Updated feedback and added screenshot.

NR for usability check.

@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 🌱 [policy, no patch] Determine a best practice for providing BC for internal paths that change between minor releases Active , 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 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work , 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 🌱 [policy, no patch] Determine a best practice for providing BC for internal paths that change between minor releases Active .
• 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".

Also soon to be renamed to Block types

So is the only blocker on this the wording?

I realise I'm a bit late to the party here, but @xjm in #16: Thanks for the info there!

I also agree with the thinking that since this is an admin facing path, removing in D11 is fine, and that if it were more user-facing then a longer window for removal makes sense.

Did not mean to change the status.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2023-01-27 Fixed . 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).

Updated message to first suggestion.

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.

Woo. As soon as this gets committed I'll update the block library ticket so we can unblock the 3 behind that.

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.

Also ran the tests locally between marking RTBC. Could it be an issue with the link (current page)

Commented in GitLab.

Updated to this page.

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 📌 Drupal Usability Meeting 2023-01-27 Fixed . (See Comment #34.) I am removing the tag for a usability review.

Commented in GitLab.

Read through the comments and it looks like all feedback has been addressed, also manually tested and updated the screenshot in the IS with the new messaging.

Marking RTBC :)

Left a comment on the MR again.

I'm happy to second RTBC status. I'm also mindful the sooner this lands the sooner we can get ✨ Move Custom block library to Content Fixed done.

Yes this has turned into a blocker for a number of tickets now.

I think we're using FormattableMarkup wrong.

I also opened 🐛 Incorrect use of FormattableMarkup in logger messages Fixed for another incorrect usage I found in core

Replaced with t()

I don't think we want that either, I'm not seeing logger messages translated in core. Happy to be wrong on that.

So just a regular string then?

Yeah, here's some examples from core

Actioned #47

Should be good now for review

No idea how we both pushed commits at the same time but looks like some other files/patch has leaked into the MR now.

Good catch, and a quick fix! :)

Not sure if this is a new restriction but it looks like the MR wants to be up to date with the base branch before being considered mergeable even if there are no conflicts. I'll leave it for now so we don't keep spinning unnecessary test runs.

Committed to 10.1.x - thanks all

larowlan → closed merge request !3244

There is still one remaining task from the issue description:

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

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.

Great to see this land, thanks everyone!

More generally, regarding only passing plain strings to logger, I wonder if we need some kind of type hinting or code sniffing to raise that in the future.

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