Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
UrlGeneratorInterface::getPathFromRoute()
is marked as @deprecated
.
However, there are places that still act on the "internal" path, especially the path alias system and other path processing.
Proposed resolution
Remove deprecation notice since we don't have a good way to replace this functionality in 8.x
Remaining tasks
Check for affected change records.
User interface changes
none
API changes
none
Comment | File | Size | Author |
---|---|---|---|
#19 | 2307061-19.patch | 928 bytes | pwolanin |
#13 | 2307061_13.patch | 9.85 KB | Mile23 |
#11 | 2307061_11.patch | 1.61 KB | Mile23 |
#3 | 2307061.patch | 1.08 KB | dawehner |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentedroughly we need a method that generates the path without running the processPath() step, and with appropriate naming and docs.
i.e. we want to get the generated but non-aliased path.
Comment #2
YesCT CreditAttribution: YesCT commentedComment #3
dawehnerIn which case, this is the patch.
Comment #4
pwolanin CreditAttribution: pwolanin commented@dawehner - however, that doesn't include the query string or fragment?
but yes, roughly it's that simple (and/or we should rename it)
Comment #5
pwolanin CreditAttribution: pwolanin commentedComment #6
jhedstromPatch here still applies, is this still relevant?
Comment #7
dawehner@jhedstrom
Afaik yes, this is still useful.
Similar to
getInternalPathFromRoute()
Comment #8
Mile23Comment #9
pwolanin CreditAttribution: pwolanin commentedGiven that we fixed the destination query string handling, I think we should instead be killing this method as indicated by the deprecation?
Comment #10
dawehnerWell, ideally we first have to get rid of all the dependent uses like
Entity::getSystemPath()
Comment #11
Mile23This patch removes all meaningful usages of
UrlGeneratorInterface::getPathFromRoute()
.There are still some test usages of
getPathFromRoute()
, but they test the method itself and not integration. We can pull those when we do the follow-up to remove the method.Comment #13
Mile23Ooooookay....
So the need for a non-aliased path is greater than
@deprecated
.This patch renames
getPathFromRoute()
togenerateInternalPathForRoute()
and fluffs out the documentation a bit.I'm 80% positive that this could all be refactored to be more elegant and beautiful, but that's not the scope of this issue.
We could re-jigger
getInternalPathFromRoute()
a bit, make it public and use it, but it's also used bygenerateFromRoute()
.So here we are.
Comment #14
pwolanin CreditAttribution: pwolanin commentedIt seems like the last patch is not in line with the goal of the issue. We really do want to remove this, so let's see why the prior patch failed.
Comment #15
Mile23There is a need to generate a non-aliased path from an internal route.
There is nothing to replace it.
The patch in #11 fails because it simply removes usages of getPathFromRoute() and replaces them with generatePath() without taking into account this need.
I can only assume that there are historic oral-history-type reasons for having all these similar-but-slightly-different methods for generating paths from routes, and I'm not up to the task of learning them.
Comment #16
pwolanin CreditAttribution: pwolanin commented@Mile23. Can you please identify where/why we generate a non-aliased path from an internal route? That is the problem to be solved here.
The patch in #11 was useful to start identifying the problems to be adderssed. Possibly this issue needs to have some sub-tasks rather than trying to address everything in one patch.
Comment #17
pwolanin CreditAttribution: pwolanin commentedI think the real issue is that path processing and aliases are coupled to the "internal" path so we cannot remove that capability from the generator if we want to be able to e.g. create a path alias for a given route.
If we are not removing it from the generator, there is no point in removing the wrapper method from Url.
Comment #18
pwolanin CreditAttribution: pwolanin commentedwrong class
Comment #19
pwolanin CreditAttribution: pwolanin commentedSimple docs fix
Comment #20
pwolanin CreditAttribution: pwolanin commentedComment #21
YesCT CreditAttribution: YesCT commentedshould @deprecated be changed to be @internal (in the code)?
#2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation has discussion about what should be internal.
guessing at my own answer: no. this is not @internal
Comment #22
pwolanin CreditAttribution: pwolanin at Acquia commented@YesCT - I'm not sure what @internal implies, but anyone creating path aliases may need to call this, which doesn't sound strictly internal
Comment #23
YesCT CreditAttribution: YesCT commented#16 mentions some separate issues might be useful, but @pwolanin says that is outdated.
so this is rtbc.
Comment #24
YesCT CreditAttribution: YesCT commentedremaining task is look for change records to be updated, but I'm not sure what to search for.
searching for getPathFromRoute
https://www.drupal.org/list-changes/published/drupal?keywords_descriptio...
gives one result
https://www.drupal.org/node/2046643
Comment #25
YesCT CreditAttribution: YesCT commentedComment #26
alexpottAfaics we have one real usage left in
Url::getInternalPath()
and that is not deprecated so I can't see anyway around this. Committed f962623 and pushed to 8.0.x. Thanks!