Problem/Motivation

Follow-up to #3339710: Disallow passing route objects to UrlGenerator methods to remove the deprecations.

Proposed resolution

Remove deprecattions from:
- #3151017: Deprecate UrlGenerator::supports() and UrlGenerator::getRouteDebugMessage()
- #3151019: Only allow route names, deprecate support for route objects in UrlGenerator methods

Add string type to $name argument of \Drupal\Core\Routing\UrlGeneratorInterface

Remaining tasks

- remove deprecations
- patch&review&commit

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3425294

Command icon Show commands

Start 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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

saurabh rawat made their first commit to this issue’s fork.

saurabh rawat opened merge request !7128

thhomas made their first commit to this issue’s fork.

thhomas’s picture

thhomas CreditAttribution: thhomas commented
Status: Active » Needs review

removed deprecation test

mstrelan’s picture

mstrelan CreditAttribution: mstrelan at PreviousNext commented
Status: Needs review » Needs work
Issue tags: +DrupalSouth 2024
thhomas’s picture

thhomas CreditAttribution: thhomas as a volunteer commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Removal appears to be correct.

Thought about expanding this to include all deprecation in Routing but that would be a lot. This is well scoped.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work

One question on the MR.

thhomas’s picture

thhomas CreditAttribution: thhomas as a volunteer commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

@thhomas could you answer the question though. Was suggested to open a follow up and maybe throw an exception.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Given we already did the deprecation I think we can just add the string type here, I don't see the need to defer that to a different issue.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Yeah #12 makes sense to me, wish I'd thought of that in the first place.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review

Fixed the above, also taken the liberty here to add array types where necessary and also implement all types that are present on the Symfony interfaces; if you were mismatching types here before you were doing something wrong already, so better to catch this in the method call than crash or do something unexpected later on.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Seems to have a Unit test failures.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review

Fixed, we extend ::generate() so it can take boolean as well as int in the third argument, fixed the signature here so this is allowed.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Removal seems good now.

  • catch committed ae31da87 on 11.x 
    Issue #3425294 by thhomas, longwave, saurabh rawat, quietone, mstrelan:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Yes this looks great, glad we added the type hints here.

Committed/pushed to 11.x, thanks!