Follow-up to #2418139: Add a toUriString method to Url class and add a route: scheme

Problem/Motivation

There may be a bug in handling a fragment of #0

Copying comment from Wim Leers:

I think there's one actual bug in the code plus 3 nitpicks that aren't worth a follow-up unless the first is going to require a follow-up:

  1. +++ b/core/lib/Drupal/Core/Url.php
@@ -240,21 +241,37 @@ public static function fromRouteMatch(RouteMatchInterface $route_match) {
+    if (!empty($uri_parts['fragment'])) {
+      $uri_options += ['fragment' => $uri_parts['fragment']];
+    }
+    unset($uri_parts['fragment']);

    Why is the unset() happening outside of the if-statement? That doesn't seem to make any sense?

    Before I file a follow-up for this tiny thing, I'd like confirmation from someone else that this is a problem. 

  2. +++ b/core/lib/Drupal/Core/Url.php
@@ -280,38 +302,68 @@ public static function fromUri($uri, $options = array()) {
+  protected static function fromEntityUri(array $uri_parts, $options, $uri) {

    Nit: This $options didn't get typehinted. 

  3. +++ b/core/lib/Drupal/Core/Url.php
@@ -358,6 +410,32 @@ protected function setUnrouted() {
+   * Return a URI string that represents tha data in the Url object.

    Nit: Returns. 

  4. +++ b/core/lib/Drupal/Core/Url.php
@@ -358,6 +410,32 @@ protected function setUnrouted() {
+   * constructed using an entity: or user-path: scheme.  A user-path: URI
+   * that does not match a Drupal route with be returned here with the base:
+   * scheme, and external URLs will be returned in their original form.

    Nit: Double space after period, 80 col formatting a bit off.

But actually - this may be a different and more real bug - should probably really use isset($uri_parts['fragment'])> there in case we are trying to jump to #0

Proposed resolution

Fix #0, add a test, fix code nits from Wim.

Remaining tasks

do it

CommentFileSizeAuthor
#9 increment.txt1.44 KBpwolanin
#9 2418613-9.patch6.83 KBpwolanin
#5 increment.txt6.69 KBpwolanin
#5 2418613-5.patch6.74 KBpwolanin
#3 2418613-3.patch1.77 KBpwolanin
#3 2418613-test-only-3.patch622 bytespwolanin

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Now that there is an issue, one more thing.

  /**
   * Generates the URI for this Url object.
   */
  public function toString() {

It was technically wrong before, but now that we have toUriString(), it definitely doesn't make sense anymore.

  1. toString() is for generating URLs.
  2. toUriString() is for generating URIs.

This is logical (because they are methods on the Url class), but the docs should reflect this.

pwolanin’s picture

pwolanin commented

working on the patch

pwolanin’s picture

pwolanin commented
Status: Active » Needs review
StatusFileSize
new 2418613-test-only-3.patch622 bytes
new 2418613-3.patch1.77 KB

Quick 1st pass at fixing the bug - doesn't address other doc/type hint concerns.

The last submitted patch, 3: 2418613-test-only-3.patch, failed testing.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 2418613-5.patch6.74 KB
new increment.txt6.69 KB

I think this fixers most of the rest and adds some more test cases.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Down to comment nits; this patch is RTBC codewise. Those comment nits are important here, because they affect the DX.

  1. +++ b/core/lib/Drupal/Core/Url.php
@@ -410,12 +415,12 @@ protected function setUnrouted() {
+   * Returns a URI string that represents tha data in the Url object.

@@ -612,7 +617,18 @@ public function setAbsolute($absolute = TRUE) {
+   * Generates the string URL representation for this Url object.

    The wording of these should be consistent. 

  2. +++ b/core/lib/Drupal/Core/Url.php
@@ -599,7 +604,7 @@ public function getUri() {
+   * Sets the absolute option value for this Url.

    "option value" sounds a bit strange? Perhaps just me. 

  3. +++ b/core/lib/Drupal/Core/Url.php
@@ -612,7 +617,18 @@ public function setAbsolute($absolute = TRUE) {
+   * For a Drupal route, or a path relative to the Drupal base URL, the returned

    "path relative to the Drupal base URL" — why not say "'user-path: URI" instead of that?

    And that'd still omit 'entity:' URIs, even though they get the same treatment.

    So what about

    For a Drupal route or an internal URI (URIs using the built-in 'user-path:' or 'entity:
 schemes), the returned …

    ?

pwolanin’s picture

pwolanin commented

re: #3 that's actually the base: case, which might have come from a user-path: but fell back to the former since there was no route.

The entity: case is not distinct since it's converted to route name and params. So basically there are 2 situations for the Url object instance for an "internal" URL - it's a route name + params, or it's using base:

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

You're right of course. But I'd say that's an implementation detail. I think the docs should explain it to the user from either the A) "route name + params" or B) "internal URI schemes" perspective. Because that's easy to understand. Knowing how those internal URI schemes are resolved exactly is not something every developer *must* know.

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new 2418613-9.patch6.83 KB
new increment.txt1.44 KB

ok, how's this?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Fix #0 bug in toUriString method in Url class » Fix #0 bug in toUriString() method in Url class, clarify toString() vs toUriString()
Status: Needs review » Reviewed & tested by the community
Issue tags: +DX (Developer Experience)

Perfect!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Committed and pushed to 8.0.x. Thanks!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

  • webchick committed b01c7c4 on 8.0.x 
    Issue #2418613 by pwolanin: Fix #0 bug in toUriString() method in Url...

Status: Fixed » Closed (fixed)

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