StaticRouteDiscoveryBase::createRoute isn't fully compatible with Symfony's Route creation since it only allows an array of methods not a single method. Since the previous route discovery created Symfony's route directly from the input, that signature was the interface and changing it to only allow array's broke existing code.
Gutenberg broke:
https://www.drupal.org/project/gutenberg/issues/3607961
Re-align core's routing discovery with Symfony's route generation. StaticRouteDiscoveryBase::createRoute() accepts string|array for both $methods and $schemes, matching Symfony's Route constructor. A kernel test covers string values for both keys in a routing.yml file.
Should Attributes align with Symfony's Route interface as well? We matched them so they are aligned now
Should the previous behavior be deprecated? Kept the behaviour for string support
None. Two parameter types on a protected method are widened to match Symfony and previous versions of Drupal.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !16231
Comments
Comment #3
neclimdulIts all route discovery, Attribute change was only the root cause.
I guess its an open question is Attribute discovery should also support strings. It seems like a convenient shortcut but I didn't make that change in the MR.
Comment #4
joelpittetWhere the problem was introduced #3311365: Use PHP attributes for route discovery
Might need a regression test, to get this committed but otherwise it’s RTBC
Comment #5
godotislateWe might as well make the same change for
schemeshere as well, since the Route constructor takesstring|array.Comment #6
godotislateComment #7
joelpittetComment #8
joelpittetFollowing pattern with title for helping people find these in the queue
Comment #9
joelpittetComment #10
godotislateI was surprised there wasn't an existing Kernel test class for routing, but OTOH it makes sense that most tests would be browser-based. I was also wondering if there should be an explicit test case for array values for
schemes:andmethods:. But this is a straightforward bug to fix, and I'm not sure that's necessary.lgtm
Comment #11
catchCommitted/pushed to main and backported to 11.x and 11.4.x ,thanks!
Comment #17
joelpittetThanks all for a quick turnaround!
My initial thought was choosing array only was the way to go, if I was choosing today but BC and parity wins for happier users