[PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider

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

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

Did my best to reroll #94 onto 11.x but the deprecation stuff is a bit new to me. It all changed in #3272779: [Symfony 6.1] Replace deprecationListenerTrait with PHPUnitBridge ignoreFile → so hopefully I've done it right.

This needed a rebase, which I did, and then updated the deprecation messages. That should help the Kernel tests to pass. I ran ManageFieldsFunctionalTest::testHelpDescriptions locally and it failed with access denied.

The problem was the requirements on the routes, which were keys which didn't exist any more -- probably from an older version of this MR, referring to requirements handlers which have been removed since.

( ✨ add a way to manually use the test site from a Functional test Needs review was useful here for debugging!)

Oh yes -- should we move most of core/modules/node/src/RouteProcessor/NodeDeprecatedRouteProcessor.php to a common class and leave just the $routeMap & the deprecation message in that class, so it can be reused in future route deprecations?

I think it would be better if we do this after 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work , with the benefit of having a lot less BC code and test coverage to maintain.

The blocker is in 📌 Support route aliasing (Symfony 5.4) and allow deprecating the route name Needs work

Updated the MR to use the new route deprecation mechanism.

Thank you! looks ready

Updated the change record a bit, removed a suggestion to create those route through a node entity by creating it, that just isn't a good idea because creating a content entity is slow.

I think it might also be useful to have a route match check as an example, because that part does _not_ have BC layer still, the current route is just the current route, it won't support the old path.

I'm also not not sure about the added ignored deprecations. I assume there's no easy way around that with update functions and what not, but that means contrib will actually not see this and it's mostly hidden. We usually only use this for third party deprecations that we don't control.

Yeah I don't think we can add those deprecations to .deprecation-ignore.txt, otherwise they will also get ignored by contrib and custom modules? What happens if we just remove that - we can hopefully work around any deprecations that are triggered at update time, and the idea is we shouldn't see any at runtime.

Removed those ignored deprecations and all the usages of the deprecated route names. Also updated the CR to mention route match checks :)

I've searched contrib usage for the node.add and node.add_page routes, and it seems they're quite popular. Should we keep them around until Drupal 13 to give folks more time to update?

Nice, seems like that was less problematic than I assumed, no update problems because it's still in a database or something. And lots of outdated uses that were hidden.

IMHO, D12 deprecation is not a huge deal, I'm much more concerned about all the cases that check the current route, because we have no BC for $routeMatch->getRouteName() == 'node.add', that's just not possible (e.g. http://codcontrib.hank.vps-private.net/node/35562366#line-138). That specific example should check if the node is new or not, but there are tons of them, in breadcrumb builders, block and alter hooks and they will all break.

because we have no BC for $routeMatch->getRouteName() == 'node.add', that's just not possible

This could probably be a Drupal phpstan rule - look for that call with a long list of deprecated routes and warn people to update.

@berdir ah yeah I bet that is going to hit a bunch of things. Not sure if this is possible but I wonder if we can teach PHPStan about route name strings (e.g. the return value of getRouteName()) and then find uses of deprecated route names?

Crossposted with @longwave in an amusing way. Let's open an issue against https://github.com/mglaman/phpstan-drupal - is that the right place?

Another option would be to load _all_ route aliases in the route match service, then in getRouteName() trigger the deprecation if the passed-in route name is an alias (and deprecated).

@catch I think that would be a place to start, we could and should also start adding these rules directly to core where it makes sense but @mglaman might have good ideas first.

a phpstan rule won't prevent that code breaks, it will just assist in updating it. But it's better than nothing.

There are lots of variations on this. If you look through the matches, there's also stuff like

$route_name = \Drupal::routeMatch()->getRouteName();
if (in_array($route_name, ['node.add', ...]))

there are switch statements with that and so on.

That's what I mean about teaching PHPStan, I don't know how far you can go but if you can tell it that the return type of getRouteName() is specifically a route-name-string (similar to how it handles class-string), if it later gets compared against a fixed set of deprecated route-name-strings can it report on that?

Discussed this a bit more in slack with @amateescu and @longwave.

We could add RouteMatch::isRouteName($route_name) or RouteMatch::isRoute($route_name), and then deprecate RouteMatch::getRouteName() altogether.

In RouteMatch::isRoute($route_name); if the route name is an alias and deprecated, we can make it work so it returns TRUE for aliased routes, but also issues a deprecation if the route is an alias and deprecated.

If we do that, we don't need a phpstan rule. Obviously the new method would only work for modules that have updated to use the new method.

I think we should open a new issue for that, and postpone this issue on that one. If we decide not to do it, we could come back here and do the phpstan rule, but it seems like a more robust way to go about things.

FWIW from Symfony 6.4 they allow autogenerated route names based on class/method names, which avoids the magic strings that we currently use: https://symfony.com/blog/new-in-symfony-6-4-fqcn-based-routes

Can we please provide more info on the DRY violation. How many methods are repeated and roughly what % of the code is repeated?

Reviewed the code. No comments added. Test-only test fails as it should. All green

Opened the issue that was proposed in #129, but I quickly found out that we can't deprecate \Drupal\Core\Routing\RouteMatchInterface::getRouteName() because there are many legitimate use cases for it, as mentioned in that issue's summary.

Which leaves us with the PHPStan rule :) Since https://github.com/mglaman/phpstan-drupal is not managed by the core team, I think that rule can't be a hard blocker for this issue..

From what I can tell all open threads are open and the follow up for checking the route opened already.

Tests appear to be passing but going to rebase as it's 100+ commits back and seems like a big shift, still passes!

Should this be highlighted 11.2 priority if it gets in?

Don't see any open threads or conversations on the MR and deprecations seem correct. Going to go on a limb and mark it