Inherits beta evaluation from parent issue: #2367749: Remove usage of deprecated UrlGeneratorInterface::generateFromPath()
Problem/Motivation
UrlGeneratorInterface::generateFromPath()
is deprecated for removal before Drupal 8.0.0 release.
UrlAlterFunctionalTest::assertUrlOutboundAlter()
and RouterTest::testUrlGeneratorFront()
call UrlGeneratorInterface::generateFromPath()
with a path instead of using a route name.
There might be other instances in the system module where UrlGeneratorInterface::generateFromPath()
is used.
These should all be removed.
Proposed resolution
Fix by changing the method call to generateFromRoute()
with a route name and parameters
Remaining tasks
User interface changes
n/a
API changes
n/a
Task | Novice task? | Contributor instructions | Complete? |
---|
Comment | File | Size | Author |
---|---|---|---|
#31 | increment.txt | 855 bytes | pwolanin |
#31 | 2558189-31.patch | 5.24 KB | pwolanin |
#27 | interdiff-2558189-22-26.txt | 1.23 KB | SteffenR |
#27 | 2558189-26.patch | 5.11 KB | SteffenR |
#23 | interdiff-2558189-16-22.txt | 1.63 KB | Cyberschorsch |
Comments
Comment #2
Mile23Comment #3
Mile23The changes for
RenderWebTest
and RouterTest are easy, thoughRouterTest
seems to have an unrelated failure locally.I ran out of time and patience (my internet is very flaky at the moment), so
UrlAlterFunctionalTest
remains unchanged.Comment #5
joshi.rohit100Its quite tricky as for assertUrlOutboundAlter as if we change this, we also (I think) need to change all the instances which calling this method.
Comment #6
JeroenTFixed exception in testUrlGeneratorFront.
I also removed the usage of generateFromPath in UrlAlterFunctionalTest but there is one test that I commented out because I don't have a clue how to convert an url like
/user/$uid/test1
to a route.Any ideas?
Comment #7
Mile23Comment #8
Mile23As it turns out, you're supposed to use the
unrouted_url_assembler
service. :-)Comment #9
Mile23Comment #10
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThere should be a method on the Url class you can use?
Look at Url::fromUri() or Url::fromUserInput()
Comment #11
JeroenTI tried the following 2 options:
But i got the error "External URLs do not have an internal route name.". I think this occurs because there is no route defined for /user/{uid}/test1.
Comment #12
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedHmm, ok - that one is going to be more complicated. I think the test is a little bogus. An option would be to test the service path_processor_manager instead of using the URL generator.
Look at the inbound test, it uses
$result = $this->container->get('path.alias_manager')->getPathByAlias($original);
So change the outbound something like this:
If that doesn't work I think Mile23 had another possible answer in #8, but it requires an extra option to be passed in that doesn't seem well documented.
see \Drupal\path\Controller\PathController::adminOverview()
Comment #13
JeroenTMade changes as suggested by pwolanin.
There are still 2 failing tests.
One of the failing tests gives the following error:
Altered outbound URL /forum, expected /community, and got /communitym.
I'm not sure where the /communitym URL comes from..
Comment #16
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedBuggy test class. Off by 1. Not sure why that passed before.
Check the diff, for community one has substr with 9 the other with 10. They should be the same. 5 vs 6 for the outbound left the "m" from forum. preg_replace() is more robust.
Comment #19
JeroenT@pwolanin, great! Thank you for your time!
Comment #20
dawehnerAt that time I would suggest going with Url::fromRoute()->setAbsolute()->toString() as alternative
This is even a bit more readable, to be honest
Comment #21
CyberschorschMe and SteffenR are going to review this issue using https://www.drupal.org/contributor-tasks/review
Comment #23
CyberschorschComment #24
CyberschorschWe applied the suggested changes from Daniel
Comment #27
SteffenRWe forgot to add ->setAbsolute()->toString() in our patch. I attached a new patch and an interdiff to the previous patch.
Comment #28
SteffenRComment #31
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedHow about like this?
Comment #32
dawehnerLooks alright for me. Its sad though that we return the docs rather than fixing the assert functions.
Comment #33
alexpottCommitted 08f9c8d and pushed to 8.0.x. Thanks!
Yep let's just return the result of
$this->assertIdentical()
here. Assertion methods should return a bool. Fixed on commit....