Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
UserRouteProvider
does not extend DefaultHtmlRouteProvider
. Thus, it contains duplicated logic.
Proposed resolution
Make UserRouteProvider
extend DefaultHtmlRouteProvider
.
Remaining tasks
Review.
User interface changes
None.
API changes
If someone has a route provider that extends UserRouteProvider
and has e.g. a getCanonicalRoute()
with different parameters than DefaultHtmlRouteProvider
this could break. I think that is something we can get away with in a minor version but I guess we could write a change notice to be nice.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff_20-21.txt | 630 bytes | pooja saraah |
#21 | 2815997-21.patch | 7.19 KB | pooja saraah |
#20 | reroll_diff_last-20.txt | 4.71 KB | sahil.goyal |
#20 | 2815997-20.patch | 7.2 KB | sahil.goyal |
user-route-provider.patch | 5.1 KB | tstoeckler | |
Comments
Comment #2
dawehnerLooks great for me
I just changed https://www.drupal.org/node/2562903/revisions/view/10118895/10149059 because I don't concrete entity handlers are part of our API. They are conceptually really similar to concrete plugins / services.
Comment #3
tstoecklerAwesome, so RTBC?
Comment #4
dawehnerOh yeah, I actually wanted to do that, but got sidetracked.
Comment #5
catchDo we want a 'to be nice' change notice for #2, or is the likelihood so low that it's unlikely to help anyone? Feels like the latter but worth asking.
Comment #6
catchI nearly committed this, but then I looked at the patch again, and it adds more code than it removes. Not only that but it's replacing mostly static YAML definitions with PHP logic.
So this doesn't really look like a net-improvement at the moment. Is there code we can just rip-out for 9.x that's added here or would that require further changes?
Comment #19
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
This seems like a good task to make things more consistent with how other entities are created on the site right?
At this time we will need a D10 version of this patch.
@alexpott unless you are saying in #6 this is a no go?
Comment #20
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedReroll the patch for drupal 10.1.x and attaching the reroll diff where i was not sure with last patch sequence number, and make few more changes in the patch to fix php coding standard issues.
Comment #21
pooja saraah CreditAttribution: pooja saraah at Srijan | A Material+ Company for Drupal India Association commentedFixed failed commands on #21
Attached patch against Drupal 10.1.x
Comment #22
longwave#21 is failing PHPStan.
@catch's comments in #6 still apply. Following #3159210: Support route aliasing (Symfony 5.4) and allow deprecating the route name we could deprecate
user.admin_create
and replace it withentity.user.add_form
which would remove some of the code, but I'm still not sure it will be worth it.