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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

roughly 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.

YesCT’s picture

Issue tags: +@deprecated
dawehner’s picture

Status: Active » Needs review
FileSize
1.08 KB

i.e. we want to get the generated but non-aliased path.

In which case, this is the patch.

pwolanin’s picture

@dawehner - however, that doesn't include the query string or fragment?

but yes, roughly it's that simple (and/or we should rename it)

pwolanin’s picture

jhedstrom’s picture

Patch here still applies, is this still relevant?

dawehner’s picture

@jhedstrom
Afaik yes, this is still useful.

Similar to getInternalPathFromRoute()

Mile23’s picture

pwolanin’s picture

Status: Needs review » Needs work

Given that we fixed the destination query string handling, I think we should instead be killing this method as indicated by the deprecation?

dawehner’s picture

Well, ideally we first have to get rid of all the dependent uses like Entity::getSystemPath()

Mile23’s picture

Title: Replace @deprecated on getPathFromRoute with an suggestion what to use as alternative. » Remove usages of deprecated UrlGeneratorInterface::getPathFromRoute()
Category: Bug report » Task
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
1.61 KB

This 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.

Status: Needs review » Needs work

The last submitted patch, 11: 2307061_11.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
9.85 KB

Ooooookay....

So the need for a non-aliased path is greater than @deprecated.

This patch renames getPathFromRoute() to generateInternalPathForRoute() 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 by generateFromRoute().

So here we are.

pwolanin’s picture

Status: Needs review » Needs work

It 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.

Mile23’s picture

There 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.

pwolanin’s picture

@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.

pwolanin’s picture

Title: Remove usages of deprecated UrlGeneratorInterface::getPathFromRoute() » Url::getInternalPath() is marked deprecated but won't be removed, so update the docs

I 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.

pwolanin’s picture

Title: Url::getInternalPath() is marked deprecated but won't be removed, so update the docs » UrlGeneratorInterface::getPathFromRoute() is marked deprecated but won't be removed, so update the docs

wrong class

pwolanin’s picture

Component: base system » documentation
Status: Needs work » Needs review
FileSize
928 bytes

Simple docs fix

pwolanin’s picture

Issue summary: View changes
YesCT’s picture

should @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

pwolanin’s picture

@YesCT - I'm not sure what @internal implies, but anyone creating path aliases may need to call this, which doesn't sound strictly internal

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

#16 mentions some separate issues might be useful, but @pwolanin says that is outdated.
so this is rtbc.

YesCT’s picture

remaining 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

YesCT’s picture

Title: UrlGeneratorInterface::getPathFromRoute() is marked deprecated but won't be removed, so update the docs » UrlGeneratorInterface::getPathFromRoute() is marked deprecated but will not be removed, so update the docs
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +rc eligible

Afaics 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!

  • alexpott committed f962623 on 8.0.x
    Issue #2307061 by Mile23, pwolanin, dawehner, YesCT:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.