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

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23’s picture

Status: Active » Needs review
FileSize
1.64 KB

The changes for RenderWebTest and RouterTest are easy, though RouterTest 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.

Status: Needs review » Needs work

The last submitted patch, 3: 2558189_3.patch, failed testing.

joshi.rohit100’s picture

Its quite tricky as for assertUrlOutboundAlter as if we change this, we also (I think) need to change all the instances which calling this method.

JeroenT’s picture

Status: Needs work » Needs review
FileSize
5.36 KB
5.67 KB

Fixed 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?

Mile23’s picture

Title: Remove usage of UrlGeneratorInterface::generateFromPath() in system module » Remove usage of deprecated UrlGeneratorInterface::generateFromPath() in system module
Mile23’s picture

As it turns out, you're supposed to use the unrouted_url_assembler service. :-)

Mile23’s picture

Status: Needs review » Needs work
pwolanin’s picture

There should be a method on the Url class you can use?

Look at Url::fromUri() or Url::fromUserInput()

JeroenT’s picture

I tried the following 2 options:

$url = Url::fromUri("internal:/user/$uid/test1");
$this->assertUrlOutboundAlter($url->getRouteName(), $url->getRouteParameters(), '/alias/test1');
$url = Url::FromUserInput("/user/$uid/test1");
$this->assertUrlOutboundAlter($url->getRouteName(), $url->getRouteParameters(), '/alias/test1');

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.

pwolanin’s picture

Hmm, 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:


  protected function assertUrlOutboundAlter($original, $final) {
    // Test outbound altering.
    $result = $this->container->get('path_processor_manager')->processOutbound($original);
    $this->assertIdentical($result, $final, format_string('Altered outbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));
  }

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()

JeroenT’s picture

Status: Needs work » Needs review
FileSize
3.11 KB
4.27 KB

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

Status: Needs review » Needs work

The last submitted patch, 13: remove_usage_of-2558189-13.patch, failed testing.

The last submitted patch, 13: remove_usage_of-2558189-13.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.92 KB
1.81 KB

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

Status: Needs review » Needs work

The last submitted patch, 16: 2558189-16.patch, failed testing.

JeroenT queued 16: 2558189-16.patch for re-testing.

JeroenT’s picture

Status: Needs work » Needs review

@pwolanin, great! Thank you for your time!

dawehner’s picture

  1. +++ b/core/modules/system/src/Tests/Common/RenderWebTest.php
    @@ -132,7 +132,7 @@ function testDrupalRenderFormElements() {
    -      ':href' => \Drupal::urlGenerator()->generateFromPath('common-test/destination', ['absolute' => TRUE]),
    +      ':href' => $this->container->get('url_generator')->generateFromRoute('common_test.destination', [], ['absolute' => TRUE]),
    
    +++ b/core/modules/system/src/Tests/Routing/RouterTest.php
    @@ -181,10 +181,10 @@ public function testControllerResolutionPage() {
    -    $this->assertEqual($this->container->get('url_generator')->generateFromPath('<front>'), $base_path);
    +    $this->assertEqual($this->container->get('url_generator')->generateFromRoute('<front>'), $base_path);
    

    At that time I would suggest going with Url::fromRoute()->setAbsolute()->toString() as alternative

  2. +++ b/core/modules/system/tests/modules/url_alter_test/src/PathProcessorTest.php
    @@ -31,9 +31,7 @@ public function processInbound($path, Request $request) {
    -    if ($path == '/community' || strpos($path, '/community/') === 0) {
    -      $path = '/forum' . substr($path, 10);
    -    }
    +    $path = preg_replace('@^/community(.*)@', '/forum$1', $path);
     
    

    This is even a bit more readable, to be honest

Cyberschorsch’s picture

Me and SteffenR are going to review this issue using https://www.drupal.org/contributor-tasks/review

SteffenR queued 16: 2558189-16.patch for re-testing.

Cyberschorsch’s picture

Cyberschorsch’s picture

We applied the suggested changes from Daniel

Status: Needs review » Needs work

The last submitted patch, 23: 2558189-22.patch, failed testing.

The last submitted patch, 23: 2558189-22.patch, failed testing.

SteffenR’s picture

We forgot to add ->setAbsolute()->toString() in our patch. I attached a new patch and an interdiff to the previous patch.

SteffenR’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: 2558189-26.patch, failed testing.

The last submitted patch, 27: 2558189-26.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
5.24 KB
855 bytes

How about like this?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks alright for me. Its sad though that we return the docs rather than fixing the assert functions.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 08f9c8d and pushed to 8.0.x. Thanks!

+++ b/core/modules/system/src/Tests/Path/UrlAlterFunctionalTest.php
@@ -82,13 +82,10 @@ function testUrlAlter() {
-   * @return
-   *   TRUE if $original was correctly altered to $final, FALSE otherwise.
...
     $this->assertIdentical($result, $final, format_string('Altered outbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));

@@ -99,8 +96,6 @@ protected function assertUrlOutboundAlter($original, $final) {
-   * @return
-   *   TRUE if $original was correctly altered to $final, FALSE otherwise.

Yep let's just return the result of $this->assertIdentical() here. Assertion methods should return a bool. Fixed on commit....

diff --git a/core/modules/system/src/Tests/Path/UrlAlterFunctionalTest.php b/core/modules/system/src/Tests/Path/UrlAlterFunctionalTest.php
index 036966b..d9a8228 100644
--- a/core/modules/system/src/Tests/Path/UrlAlterFunctionalTest.php
+++ b/core/modules/system/src/Tests/Path/UrlAlterFunctionalTest.php
@@ -82,11 +82,14 @@ function testUrlAlter() {
    *   A string with the original path that is run through generateFrommPath().
    * @param $final
    *   A string with the expected result after generateFrommPath().
+   *
+   * @return
+   *   TRUE if $original was correctly altered to $final, FALSE otherwise.
    */
   protected function assertUrlOutboundAlter($original, $final) {
     // Test outbound altering.
     $result = $this->container->get('path_processor_manager')->processOutbound($original);
-    $this->assertIdentical($result, $final, format_string('Altered outbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));
+    return $this->assertIdentical($result, $final, format_string('Altered outbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));
   }
 
   /**
@@ -96,10 +99,13 @@ protected function assertUrlOutboundAlter($original, $final) {
    *   The original path before it has been altered by inbound URL processing.
    * @param $final
    *   A string with the expected result.
+   *
+   * @return
+   *   TRUE if $original was correctly altered to $final, FALSE otherwise.
    */
   protected function assertUrlInboundAlter($original, $final) {
     // Test inbound altering.
     $result = $this->container->get('path.alias_manager')->getPathByAlias($original);
-    $this->assertIdentical($result, $final, format_string('Altered inbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));
+    return $this->assertIdentical($result, $final, format_string('Altered inbound URL %original, expected %final, and got %result.', array('%original' => $original, '%final' => $final, '%result' => $result)));
   }
 }

  • alexpott committed 08f9c8d on 8.0.x
    Issue #2558189 by JeroenT, pwolanin, Cyberschorsch, SteffenR, Mile23:...

Status: Fixed » Closed (fixed)

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