This is a child issue of #2339219: [meta] Finalize URL generation API (naming, docs, deprecation). See that issue's summary for additional context.

Problem/Motivation

  • In Drupal 7, url() and l() could be passed one of:
    • an internal Drupal path (e.g., node/1)
    • a relative URL to a resource on the same machine but that's not an internal Drupal path (e.g., /robots.txt)
    • an absolute URL (e.g., http://example.com/foo/bar)
  • In Drupal 8, internal Drupal paths are a deprecated concept, replaced by route names and parameters.
  • The Drupal 8 replacements to url() and l() are methods of the same name within UrlGeneratorTrait and LinkGeneratorTrait. However, these, by design, only accept route names and parameters; they do not accept non-Drupal-routed URLs (relative or absolute) as arguments.
  • #2343661: Rename l() to _l() and url() to _url(), and document replacements will remove the global url() and l() functions.
  • So, what are D7 modules that are currently calling url()/l() for non-Drupal-routed URLs supposed to do when they port to D8?

Proposed resolution

  • Create a new service that handles building a URL given either a relative or absolute URL. It can add query string, fragment, etc, will validate the scheme, and pre-pend the base path. It will mostly be copying the code from UrlGenerator::generateFromPath(), but omitting the processPath() call since we don't want to try to alias or do language processing
  • Have separate methods for the validating/sanitizing the scheme vs handling the options, and a convenience one that calls both
  • Replace calls in core to url() with actual URLs to calls to this service
  • Inject this service into the UrlGenerator to avoid code duplication
  • Remove UrlGenerator::generateFromPath()

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#86 interdiff.txt1.94 KBtim.plunkett
#86 2343759-url-from-methods-86.patch57.94 KBtim.plunkett
#84 interdiff.txt24.9 KBtim.plunkett
#84 2343759-url-from-methods-84.patch57.94 KBtim.plunkett
#79 2343759-url-79.patch49.58 KBtim.plunkett
#76 interdiff.txt3.29 KBeffulgentsia
#76 2343759-75.patch53.27 KBeffulgentsia
#73 2343759-73.patch48.06 KBdawehner
#71 interdiff.txt2.05 KBdawehner
#71 2343759-71.patch48.03 KBdawehner
#67 interdiff.txt3.15 KBdawehner
#67 2343759-67.patch47.15 KBdawehner
#65 interdiff.txt591 bytesdawehner
#65 2343759-65.patch314.97 KBdawehner
#64 interdiff-url-docs.txt5.7 KBxjm
#64 url-2343759-63-do-not-test.patch47.78 KBxjm
#60 2343759-60.patch45.54 KBdawehner
#60 interdiff.txt3.73 KBdawehner
#57 2343759-57.patch46.66 KBpwolanin
#52 2343759-52.patch46.53 KBpwolanin
#52 increment.txt10.23 KBpwolanin
#50 interdiff.txt9.83 KBWim Leers
#47 interdiff.txt2.21 KBlarowlan
#44 2343759-44.patch39.97 KBpwolanin
#44 increment.txt2.87 KBpwolanin
#41 url-l-external-2343759.41.patch39.94 KBlarowlan
#40 url-l-external-2343759.40ish.patch134.52 KBlarowlan
#40 interdiff.txt1.69 KBlarowlan
#36 url-l-external-2343759.36.patch38.81 KBlarowlan
#36 interdiff.txt1.93 KBlarowlan
#35 url-l-external-2343759.35.patch37.1 KBlarowlan
#35 interdiff.txt819 byteslarowlan
#34 url-l-external-2343759.35.patch36.99 KBlarowlan
#34 interdiff.txt10.57 KBlarowlan
#31 2343759-31.patch43.15 KBpwolanin
#31 increment.txt637 bytespwolanin
#28 2343759-28.patch42.53 KBpwolanin
#28 increment.txt913 bytespwolanin
#26 2343759-26.patch42.32 KBpwolanin
#20 2343759-20.patch14.07 KBpwolanin
#20 increment.txt4.33 KBpwolanin
#19 2343759-19.patch13.53 KBpwolanin
#16 2343759-16.patch6.59 KBpwolanin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Issue summary: View changes
dawehner’s picture

Had some idea in mind:

  • generateFromExternalPath() (exactly the same as generateFromUriReference())
  • generateFromDomainInternalPath() (relative paths not controlled by Drupal, which I think is a really important usecase in the world, given JS frameworks/other software)

I really like the fact that standard based approach, though the question is what do people aspect. The question is which way, do we want to go.

What do you think about the domain path bit (other ideas, I think it is though needed)

Leave generateFromInternalPath() as @deprecated, same as generateFromPath() currently is.

100% agreed

effulgentsia’s picture

Is there a reason to split generateFromUriReference() into generateFromExternalPath() and generateFromDomainInternalPath()? I was envisioning generateFromUriReference() as being able to support both, since "URI references" can be either relative to hostname or absolute. Or, do you see /update.php and http://example.com/foo/bar as being sufficiently different to warrant separate methods?

effulgentsia’s picture

Issue summary: View changes

Or, do you see /update.php and http://example.com/foo/bar as being sufficiently different to warrant separate methods?

Oops. Of course, I had to pick as an example something that we recently changed from a non-Drupal-path to a Drupal-path (#2250119: Run updates in a full environment). Updated the issue summary to use /robots.txt as the example of an "external" path on the same host.

catch’s picture

install.php would be another example.

dawehner’s picture

@effulgentsia
This wasn't clear for me at least from your IS. I would be fine with both, the question is what people would expect.
In general it helps for me as a developer if I have one method which just does one thing, but this one thing might be using a UriReference() so I think having one would be fine.

dawehner’s picture

Adding a related issue

martin107’s picture

Just a minor thing..

Yesterday, in IRC I saw lots of frustration and gnashing of teeth over PHP and its handling of _toString() errors.

This made me feel a little guilty because, under the 'Probably bug' section of my lint tool I had seen pile of lint error in this regard and done nothing

\Drupal\core\Url is on the list.

From batch.inc

  $build = array(
    '#theme' => 'progress_bar',
    '#percent' => $percentage,
    '#message' => $message,
    '#label' => $label,
    '#attached' => array(
      'drupal_add_html_head' => array(
        array(
          array(
            // Redirect through a 'Refresh' meta tag if JavaScript is disabled.
            '#tag' => 'meta',
            '#noscript' => TRUE,
            '#attributes' => array(
              'http-equiv' => 'Refresh',
              'content' => '0; URL=' . $url,   <--- this line
            ),
          ),
          'batch_progress_meta_refresh',
        ),
      ),
dawehner’s picture

@martin107

\Drupal\Core\Url provides toString() but not __toString()

martin107’s picture

I see that, my thinking was flawed in other ways ....

the fix is trivial, but I thought it was going to be washed out with the changes here and in others in the works.

but what ever way I cut this function the change will remain relatively constant

 function _batch_progress_page() {
             '#noscript' => TRUE,
             '#attributes' => array(
               'http-equiv' => 'Refresh',
-              'content' => '0; URL=' . $url,
+              'content' => '0; URL=' . $url->toString(),
             ),
           ),
           'batch_progress_meta_refresh',

So sorry for the noise the thing to do it put it forward as a quickfix child of #2344799: [Meta issue] Clear _toString is not implemented errors.

dawehner’s picture

@martin107
In HEAD $url is a string.

xjm’s picture

So I thought I posted a comment on this issue Wednesday, but it seems to have been lost in the ether.

I'd prefer to have separate methods for internal, non-routed vs. external URLs. Here's why: As a module author, I can easily understand what an external URL is. I can also fairly easily understand what a "path processed by Drupal" is. The third sub-category of "what url() used to do"--these internal, non-routed paths like /robots.txt--is a bit less obvious, but I can figure it out from the first two.

However, if the first and third things are lumped together, that makes it a lot more confusing to understand what I'm supposed to use when, and it would be easy to try to misuse that method for internal, routed paths.

effulgentsia’s picture

Ok, so if we do #12, what names?

How about generateFromExternalPath() for things like /install.php and /robots.txt? It's "external" in the sense of being external to Drupal/Symfony routing, which is where UrlGeneratorInterface lives. And the word "path" means internal to the domain, per http://en.wikipedia.org/wiki/Uniform_resource_locator#Syntax.

And for passing a URL rather than just a path, how about either generateFromUrl() or sanitizeUrl()? Neither name is great, because it's kind of weird to "generate a URL from a URL", but the function does more than just sanitize (e.g., $options can include query and fragment additions).

Better ideas for either welcome.

pwolanin’s picture

Discussing with dawehner and effulgentsia - let's take a different approach and create a tiny service that just does URL "tweaking". Will update the summary to reflect that.

pwolanin’s picture

Assigned: Unassigned » pwolanin
Issue summary: View changes
Issue tags: -Needs issue summary update
pwolanin’s picture

Status: Active » Needs work
FileSize
6.59 KB

Here's a quick first pass at building a class and interface so we can argue about naming, etc with something to look at.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Routing/UrlBuilder.php
@@ -0,0 +1,153 @@
+  public function buildFromPath($path= NULL, array $options = array()) {
...
+  public function buildExternalUrl($path = NULL, array $options = array()) {
...
+  public function buildLocalUrl($path = NULL, array $options = array()) {

I wonder whether we should still have some protected against accidentally passing in a url object?

xjm’s picture

I don't understand what buildFromPath() is supposed to do from the name (with no docs or explanation on-isue), so maybe that's an indication that it needs a different name?

I'm assuming buildLocalUrl() is robots.txt and buildExternalUrl() is http://othersite.example.com/foo?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.53 KB

Here's a more functional patch after more discussion with dawehner and Wim Leers - we need a way to indicate that the Drupal based path should be inserted - use a special case scheme "base://"

Let's see what the tests say.

pwolanin’s picture

FileSize
4.33 KB
14.07 KB

And a few more tweaks for better security.

dawehner’s picture

I really like the simplification needed here.

  1. +++ b/core/lib/Drupal/Core/Routing/UrlBuilder.php
    @@ -0,0 +1,135 @@
    +/**
    + * Drupal-specific URL Matcher; handles the Drupal "system path" mapping.
    + */
    

    Let's ensure to not forget to fix this.

  2. +++ b/core/lib/Drupal/Core/Routing/UrlBuilder.php
    @@ -0,0 +1,135 @@
    +      // UrlHelper::isExternal() only returns true for safe protocols.
    +      return $this->buildExternalUrl($path, $options, FALSE);
    

    I really wonder whether we should try to performance-optimize here.

  3. +++ b/core/lib/Drupal/Core/Routing/UrlBuilder.php
    @@ -0,0 +1,135 @@
    +    // Split off the fragment.
    +    if (strpos($path, '#') !== FALSE) {
    +      list($path, $old_fragment) = explode('#', $path, 2);
    +      // If $options contains no fragment, take it over from the path.
    +      if (isset($old_fragment) && !$options['fragment']) {
    +        $options['fragment'] = '#' . $old_fragment;
    +      }
    +    }
    

    I am curious whether we want to support this automatic extraction on the longrun. Was is the point in here? (Sure this is just a copy of the old code).

  4. +++ b/core/lib/Drupal/Core/Routing/UrlBuilderInterface.php
    @@ -0,0 +1,117 @@
    +
    +  /**
    +   * Builds a domain-local or external URL from a path or URL.
    +   *
    +   * This is a helper function that calls buildExternalUrl() or buildLocalUrl()
    +   * based on a check of whether the path is a valid external URL.
    

    I think we should recommend actively people to use the other method, if they know what they want to do.

xjm’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/UrlBuilder.php
    @@ -0,0 +1,135 @@
    +namespace Drupal\Core\Routing;
    

    Is this supposed to be in the routing component? As far as I know it explicitly does not depend on the routing system, no? Unless we're going to merge this with something that supports routes, or...

  2. +++ b/core/lib/Drupal/Core/Routing/UrlBuilder.php
    @@ -0,0 +1,135 @@
    + * Drupal-specific URL Matcher; handles the Drupal "system path" mapping.
    + */
    +class UrlBuilder implements UrlBuilderInterface {
    
    +++ b/core/lib/Drupal/Core/Routing/UrlBuilderInterface.php
    @@ -0,0 +1,117 @@
    +interface UrlBuilderInterface {
    

    From this, I have absolutely no idea why there is a UrlBuilder and a UrlGenerator. That is going to cause a lot of frustration, especially since people need to know about both of them for porting url() from D7.

  3. +++ b/core/lib/Drupal/Core/Routing/UrlBuilder.php
    @@ -0,0 +1,135 @@
    +    if (strpos($path, 'base://') !== 0 && UrlHelper::isExternal($path)) {
    ...
    +    if (strpos($path, 'base://') === 0) {
    
    +++ b/core/lib/Drupal/Core/Routing/UrlBuilderInterface.php
    @@ -0,0 +1,117 @@
    +   *   A path on the same domain. If the path is prefixed with with base://,
    +   *   the Drupal base path with be included. This only matters if Drupal is
    +   *   installed in a subdomain.
    

    "the Drupal base path will be included in the generated URL," I guess? Still on the fence about this.

  4. +++ b/core/lib/Drupal/Core/Routing/UrlBuilderInterface.php
    @@ -0,0 +1,117 @@
    +   * Builds a domain-local or external URL from a path or URL.
    +   *
    +   * This is a helper function that calls buildExternalUrl() or buildLocalUrl()
    +   * based on a check of whether the path is a valid external URL.
    

    We need some documentation near the top here (similar to in the original patch from #2339219: [meta] Finalize URL generation API (naming, docs, deprecation)) that emphasizes that this should not be used for pages generated by Drupal (i.e. handled by the routing system). Also with @see to the UrlGenerator for those.

  5. +++ b/core/lib/Drupal/Core/Routing/UrlBuilderInterface.php
    @@ -0,0 +1,117 @@
    +  public function buildFromPath($path, array $options = array());
    

    Still not sure about this name. I guess buildLocalOrExternalUrl() is too verbose?

    Or, it's already called UrlBuilder. Could we do instead:

    • UrlBuilder::localUrl()
    • UrlBuilder::externalUrl()
    • UrlBuilder::localOrExternalUrl()
xjm’s picture

@pwolanin, have you reviewed UrlGenerator? I don't understand why we are putting these methods on a different, almost identically named class.

pwolanin’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
42.32 KB

See the meta issue for details until the issue summary is updated here.

Updated patch that will hopefully run at least. No interdiff, since it's a rather different approach. Note that this includes dawehner's work on #2346361: Add a UnroutedUrlAssembler and put it into the container

pwolanin’s picture

Status: Needs work » Needs review
FileSize
913 bytes
42.53 KB

Fix missing use

pwolanin’s picture

Issue tags: +Amsterdam2014
pwolanin’s picture

Status: Needs work » Needs review
FileSize
637 bytes
43.15 KB

oops, missed converting a method call.

larowlan’s picture

Assigned: pwolanin » larowlan

/me takes the baton
timezones++ (for once)

larowlan’s picture

Status: Needs work » Needs review
FileSize
10.57 KB
36.99 KB

phpunit passing locally, expecting fails but not sure how many - let the bot do its thing

larowlan’s picture

fixes shortcut

larowlan’s picture

More fixes

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
1.69 KB
134.52 KB

screwy stuff going on in batch redirect for installer, hoping this is enough to sort it.
got to look at some other stuff will check back later
install test stuff passing locally, but not sure what else this will break

larowlan’s picture

last patch wasn't merged against HEAD

larowlan’s picture

Green!

pwolanin’s picture

FileSize
2.87 KB
39.97 KB

minor doc clean-ups and add one useful method (but not used currently)

xjm’s picture

+++ b/core/includes/batch.inc
@@ -452,11 +452,17 @@ function _batch_finished() {
+      if (UrlHelper::isExternal($options['path']) || strpos($options['path'], 'base://') === 0) {

I think we should have one or more of isRouted(), isUnrouted(), isUri(), and/or isInternal() methods rather than doing string parsing magic. @Crell pointed out that they maybe should not be on UrlHelper, but that is out of scope here; for now, we should put them on the same class as isExternal().

xjm’s picture

+++ b/core/includes/install.core.inc
@@ -543,7 +543,7 @@ function install_run_task($task, &$install_state) {
-      $response = batch_process(install_redirect_url($install_state), install_full_redirect_url($install_state));
+      $response = batch_process('base://' . install_redirect_url($install_state), install_full_redirect_url($install_state));

So this is... suspicious. If we need to concatenate base:// on here, are other uses of install_redirect_url() elsewhere broken?

larowlan’s picture

FileSize
2.21 KB

there are no other uses that also use batch_process which in turn passes it to ::unrouted

this fixes linkformatter changes, no patch

xjm’s picture

We had some reservations about where we're putting these classes and the dependencies we're adding, so I filed: #2346683: Decide where to put URL and link generation classes for decoupling/dependencies.

We should be able to clean it up in a mostly-BC way later, so we don't need to worry about it now.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I've also been working on a full review. I have a bunch of fixed nitpicks, a bug that I added test coverage for and I discovered the same problem as #46.

  • #45: +1
  • #46: I'm debugging this *right now*!

Reroll coming very soon.

Wim Leers’s picture

  1. +++ b/core/includes/install.core.inc
    @@ -850,7 +850,7 @@ function install_get_form($form_id, array &$install_state) {
    -  return 'core/install.php?' . UrlHelper::buildQuery($install_state['parameters']);
    +  return 'install.php?' . UrlHelper::buildQuery($install_state['parameters']);
     }
    

    Turns out that "domain/install.php" automatically redirects to "domain/core/install.php", so this wouldn't cause any problems, hence it would only slow down the installation unnecessary. So this was a regression, but a fairly harmless one. Nevertheless, reverted.

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -28,6 +28,13 @@ class Url {
    +   * The URL builder.
    

    unrouted URL assembler (Fixed!)

  3. +++ b/core/lib/Drupal/Core/Url.php
    @@ -56,20 +63,27 @@ class Url {
    -  protected $path;
    +  protected $uri;
    

    Yes! :)

  4. +++ b/core/lib/Drupal/Core/Url.php
    @@ -103,44 +117,81 @@ public function __construct($route_name, $route_parameters = array(), $options =
    +   * Creates a new routed (internal to Drupal) URL object.
    ...
    +   *   A new routed (internal to Drupal) URL object.
    ...
    +   * Creates a new unrouted (external to Drupal) URL object.
    ...
    +   *   A new unrouted (external to Drupal) URL object.
    

    s/URL/Url/ (Fixed!)

  5. +++ b/core/lib/Drupal/Core/Url.php
    @@ -163,23 +214,20 @@ public static function createFromRequest(Request $request) {
    +   * Sets this Url to encapselate an unrouted URI.
    

    s/encapselate/encapsulate/ (Fixed!)

  6. +++ b/core/lib/Drupal/Core/Url.php
    @@ -163,23 +214,20 @@ public static function createFromRequest(Request $request) {
    +    // What was passed in as the route name is actually the uri.
    

    s/uri/URI/ (Fixed!)

  7. +++ b/core/lib/Drupal/Core/Url.php
    @@ -363,7 +417,7 @@ public function toString() {
       public function toArray() {
         if ($this->isExternal()) {
           return array(
    -        'path' => $this->getPath(),
    +        'path' => $this->getUri(),
    

    Isn't this wrong? Unrouted URIs that aren't external (e.g. base://robots.txt) would still be mapped to the code path for routes.

    Updated the test coverage, confirmed the bug, fixed it.

  8. +++ b/core/lib/Drupal/Core/Url.php
    @@ -383,9 +437,9 @@ public function toArray() {
    -    if ($this->isExternal()) {
    +    if ($this->unrouted) {
    

    Here it is updated correctly :)

  9. +++ b/core/lib/Drupal/Core/Url.php
    @@ -408,13 +462,13 @@ public function toRenderArray() {
           throw new \UnexpectedValueException('External URLs do not have internal representations.');
    

    s/External URLs/Unrouted URIs/ (Fixed!)

  10. +++ b/core/lib/Drupal/Core/Url.php
    @@ -473,6 +527,19 @@ protected function urlGenerator() {
    +   * Gets the URL builder for non-Drupal URLs.
    ...
    +   *   The URL builder.
    

    s/URL builder/unrouted URL assembler/ (Fixed!)

  11. +++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
    index 2d37566..4fb93b2 100644
    --- a/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    
    --- a/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    
    +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -198,7 +198,7 @@ public function extractFormValues(array &$form, FormStateInterface $form_state)
    
    @@ -198,7 +198,7 @@ public function extractFormValues(array &$form, FormStateInterface $form_state)
     
         if ($extracted) {
           if ($extracted->isExternal()) {
    -        $new_definition['url'] = $extracted->getPath();
    +        $new_definition['url'] = $extracted->getUri();
           }
    

    This means that base:// URLs (like base://robots.txt) won't work for menu links.

    Furthermore, PathValidator::getUrlIfValid() (called just before this snippet of code) doesn't handle base:// URLs.

    Should that be a follow-up?

The last point is the only one that still needs to be answered before this is RTBC IMO.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
pwolanin’s picture

FileSize
10.23 KB
46.53 KB

We've been working in parallel. Here's our added fixes on top of What Wim added.

pwolanin’s picture

@Wim Leers - links like 'base://robots.txt' are not expected to work for menu link. I don't think they should. We can debate as a follow-up, but honestly, I think it would be horribly abused.

Status: Needs work » Needs review

pwolanin queued 52: 2343759-52.patch for re-testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
46.66 KB

missed a rename in Wim's patch

diff --git a/core/tests/Drupal/Tests/Core/ExternalUrlTest.php b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
similarity index 100%
rename from core/tests/Drupal/Tests/Core/ExternalUrlTest.php
rename to core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
piper:drupal-8 pwolanin$ git ci -am"rename file"

xjm’s picture

Dreditor started going crazy so pasting my notes here. I will incorporate these docs changes into the sandbox. Review was based on #44 and I was in the middle of reviewing Url.

  1. +++ b/core/includes/install.core.inc
    @@ -850,7 +850,7 @@ function install_get_form($form_id, array &$install_state) {
    -  return 'core/install.php?' . UrlHelper::buildQuery($install_state['parameters']);
    +  return 'install.php?' . UrlHelper::buildQuery($install_state['parameters']);
    

    This also is questionable, but @dawehner says @pwolanin is fixing it. :)

  2. +++ b/core/lib/Drupal/Core/Url.php
    @@ -28,6 +28,13 @@ class Url {
    +   * The URL builder.
    

    "The URL assembler"

  3. +++ b/core/lib/Drupal/Core/Url.php
    @@ -56,20 +63,27 @@ class Url {
    -   * The external path.
    +   * Indicates whether this URL is a URI instead of a route.
    

    So, better would be:

    Indicates whether this is the URL for a non-routed URI.

    Because URIs are not routes.

  4. +++ b/core/lib/Drupal/Core/Url.php
    @@ -56,20 +63,27 @@ class Url {
    +   * The non-route uri.
    

    Nitpick: uri should be caps.

  5. +++ b/core/lib/Drupal/Core/Url.php
    @@ -103,44 +117,81 @@ public function __construct($route_name, $route_parameters = array(), $options =
    -   * Returns the Url object matching a path. READ THE FOLLOWING SECURITY NOTE.
    

    We should add a separate paragraph here that says something like:

    This method is for URLs that have Drupal routes (that is, most pages generated by Drupal). For non-routed local paths relative to the base path (like robots.txt) use Url::unrouted() with the base:// scheme.

    Then we add an @see to unrouted().

  6. +++ b/core/lib/Drupal/Core/Url.php
    @@ -103,44 +117,81 @@ public function __construct($route_name, $route_parameters = array(), $options =
    +   * @param array $route_parameters
    +   *   (optional) An associative array of parameter names and values.
    

    An associative array of route parameter names and values.

  7. +++ b/core/lib/Drupal/Core/Url.php
    @@ -103,44 +117,81 @@ public function __construct($route_name, $route_parameters = array(), $options =
    +   *   (optional) An associative array of additional options, with the following
    +   *   elements:
    

    An associative array of additional URL options

  8. +++ b/core/lib/Drupal/Core/Url.php
    @@ -103,44 +117,81 @@ public function __construct($route_name, $route_parameters = array(), $options =
    +   *   A new routed (internal to Drupal) URL object.
    ...
    +   *   A new unrouted (external to Drupal) URL object.
    

    So, the object is not routed. That would be weird. How about:

    A new URL object for a routed (internal to Drupal) URL.

    and

    A new URL object for an unrouted (non-Drupal) URL object.

    Same for the docblocks I guess.

  9. +++ b/core/lib/Drupal/Core/Url.php
    @@ -103,44 +117,81 @@ public function __construct($route_name, $route_parameters = array(), $options =
    +   * Creates a new unrouted (external to Drupal) URL object.
    

    Similar to the above, add a paragraph:

    This method is for generating URLs for URIs that do not have Drupal routes, both external URLs and unrouted local URLs like base://robots.txt. For URLs for routed paths (that is, pages generated by Drupal), use Url::routed().

    and a similar @see.

  10. +++ b/core/lib/Drupal/Core/Url.php
    @@ -163,23 +214,20 @@ public static function createFromRequest(Request $request) {
    -   * Sets this Url to be external.
    +   * Sets this Url to encapselate an unrouted URI.
    

    Spelling: encapsulate.

  11. +++ b/core/lib/Drupal/Core/Url.php
    @@ -163,23 +214,20 @@ public static function createFromRequest(Request $request) {
    +    // What was passed in as the route name is actually the uri.
    

    URI in caps (here and elsewhere).

    Should we just change the name of the parameter and constructor argument to (e.g.) $route_or_uri? Could be in a followup since this is a pre-existing problem that just becomes weirder.

  12. +++ b/core/lib/Drupal/Core/Url.php
    @@ -163,23 +214,20 @@ public static function createFromRequest(Request $request) {
    +    if ($this->external = strpos($this->uri, 'base://') !== 0) {
    

    This is another place we want the new method (as above) instead of the strpos; could be a followup.

    Other name idea: isBaseRelativeUri()....

  13. +++ b/core/lib/Drupal/Core/Url.php
    @@ -312,22 +375,22 @@ public function setOption($name, $value) {
    +   * Returns the URI of the URL.
    

    How about "..of the unrouted URL."

dawehner’s picture

FileSize
3.73 KB
45.54 KB

Addressing my own points.

webchick’s picture

Status: Needs review » Needs work

According to https://qa.drupal.org/pifr/test/875118 this failed testbot. :(

Crell’s picture

I know there's still work happening but this looks mostly fine to me. Just a few smaller critiques below.

(I would still favor fromUnrouted() and fromRouted() or such.)

  1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkBase.php
    @@ -141,9 +141,7 @@ public function getUrlObject($title_attribute = TRUE) {
    +      return Url::unrouted($this->pluginDefinition['url'], $options);;
    

    Extra ;

  2. +++ b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
    @@ -34,11 +34,18 @@ class ExternalUrlTest extends UnitTestCase {
    +  protected $unrouted_external = 'http://drupal.org';
    +
    +  /**
    +   * An unrouted, internal URL to test.
    +   *
    +   * @var string
    +   */
    +  protected $unrouted_internal = 'base://robots.txt';
    

    Coding standards: These should be lowerCamel.

  3. +++ b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
    @@ -118,12 +134,18 @@ public function testToString(Url $url) {
    -  public function testToArray(Url $url) {
    +  public function testToArray(array $urls) {
         $expected = array(
    

    I'm unclear from context why we're moving this to an array of urls? Is there a @dataProvider on it? If not,should there be?

xjm’s picture

Attached is just some docs cleanups that I've pushed to the sandbox. It does not address #63 or the test failure, so uploading as a do-not-test patch since we know it would fail.

dawehner’s picture

Status: Needs work » Needs review
FileSize
314.97 KB
591 bytes

I'm STUPID

dawehner’s picture

Status: Needs work » Needs review
FileSize
47.15 KB
3.15 KB

I'm unclear from context why we're moving this to an array of urls? Is there a @dataProvider on it? If not,should there be?

This is using a a @depends annotation so this is passed along from the previous tests etc.

Wim Leers’s picture

Indeed, it uses @depends. I also was confused by that, but this is 1) the pre-existing pattern, 2) exactly the same technique (array of Url objects) is used in the UrlTest sibling-test.

xjm’s picture

+++ b/core/lib/Drupal/Core/Url.php
@@ -103,44 +127,93 @@ public function __construct($route_name, $route_parameters = array(), $options =
+  public static function unrouted($uri, $options = array()) {
+    $url = new static($uri, array(), $options);
+    $url->setUnrouted();
+    return $url;

@@ -163,23 +236,23 @@ public static function createFromRequest(Request $request) {
   /**
-   * Sets this Url to be external.
+   * Sets this Url to encapsulate an unrouted URI.
    *
    * @return $this
    */
-  protected function setExternal() {
-    $this->external = TRUE;
-
-    // What was passed in as the route name is actually the path.
-    $this->path = $this->routeName;
-
+  protected function setUnrouted() {
+    $this->unrouted = TRUE;
+    // What was passed in as the route name is actually the URI.
+    // @todo Consider fixing this in https://www.drupal.org/node/2346787.
+    $this->uri = $this->routeName;
     // Set empty route name and parameters.
     $this->routeName = NULL;
     $this->routeParameters = array();
-    // Flag the path as external so the UrlGenerator does not need to check.
-    $this->options['external'] = TRUE;
-
-    return $this;
+    // @todo Add a method for the check below in
+    // https://www.drupal.org/node/2346859.
+    if ($this->external = strpos($this->uri, 'base://') !== 0) {
+      $this->options['external'] = TRUE;
+    }
   }

We need to throw an exception here (either in Url::unrouted() or Url::setUnrouted()) if someone tries to pass a string that does not have a scheme. The exception should tell the developer that Url::unrouted() is only for non-routed, non-Drupal URLs, that routed URLs should use a route name with Url::routed(), and that non-routed base-relative URIs should begin with the base:// prefix. See the second paragraph of the doblock on Url::unrouted() for a starting point.

Edit: Also we need test coverage for throwing said exception.

Wim Leers’s picture

Right, we forgot that! Great catch! :)

dawehner’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
48.06 KB

Let's reroll.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -196,11 +196,19 @@ public static function routed($route_name, $route_parameters = array(), $options
    +   *   Thrown when the passed in path has no schema.
    
    +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssemblerInterface.php
    @@ -49,6 +49,9 @@
    +   *   Thrown when the passed in path has no schema.
    

    s/schema/scheme/

webchick’s picture

I can fix that on commit. :) Any other feedback, or is this RTBC?

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Url.php
@@ -361,9 +457,9 @@ public function toString() {
-        'path' => $this->getPath(),
+        'path' => $this->getUri(),
...
+++ b/core/lib/Drupal/Core/Url.php
@@ -383,9 +479,9 @@ public function toArray() {
-        '#href' => $this->getPath(),
+        '#href' => $this->getUri(),

Both of these return a URI to a consumer that isn't handling a URI successfully, but that's an edge case, and makes more sense to address in follow ups, so filed those and added @todos. Also fixed #74 and another text change Wim found and told me about as I was rolling this.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Wonderful! :)

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
49.58 KB
tim.plunkett’s picture

I was not involved in any of the discussions about naming, but I really don't like that "unrouted" is exposed as a public API.

We have two static factory methods. One is when you have a route name, and the other a URI.
Why not call them ::fromRoute() or ::fromRouteName(), and ::fromUri()? Internally they can be routed/unrouted.

catch’s picture

Status: Reviewed & tested by the community » Needs work

OK this looks great but after discussion with Alex Pott we're both not keen on routed vs. unrouted. The idea came up to use fromUri() and fromRoute() instead - now that we have the base::// scheme that seems fine. We could also improve the exception method in fromUri() to point to fromRoute() when there's no scheme passed in.

Discussed this with webchick, xjm and effulgentsia as well and they agree with that change. So sorry to CNW but unless there's a strong objection could we rename the methods?

catch’s picture

Crossposted with Tim, but yes, exactly.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Working on this.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs work » Needs review
FileSize
57.94 KB
24.9 KB

Made the changes. I also expanded the exception message in fromUri().

Finally, cleaned up the @covers annotations in the tests, they were referencing old methods. In doing so I noticed we had a redundant test method, which I've removed.

larowlan’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/Url.php
    @@ -199,11 +199,11 @@ public static function routed($route_name, $route_parameters = array(), $options
    +      throw new \InvalidArgumentException('You must use a valid URI scheme. Use base:// for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controller by Drupal.');
    

    %s/controller/controlled

  2. +++ b/core/tests/Drupal/Tests/Core/UnroutedUrlTest.php
    @@ -34,14 +34,14 @@ class UnroutedUrlTest extends UnitTestCase {
    +   * An fromUri, external URL to test.
    ...
    +   * An fromUri, internal URL to test.
    

    %s/An/A

  3. +++ b/core/tests/Drupal/Tests/Core/UrlTest.php
    @@ -207,30 +189,30 @@ public function testIsExternal($urls) {
    +  public function _testGetUriForInternalUrl($urls) {
    

    this drops the test right? think that needs to go back in some form

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
57.94 KB
1.94 KB
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

back to rtbc unless bot says otherwise

larowlan’s picture

Updated change record to reflect new method names ::fromRoute and ::fromUri

pwolanin’s picture

better names, thanks, +1

Crell’s picture

fromUri()/fromRoute()++

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3102292 and pushed to 8.0.x. Thanks!

  • alexpott committed 3102292 on 8.0.x
    Issue #2343759 by pwolanin, larowlan, dawehner, tim.plunkett,...
geerlingguy’s picture

I updated the change record and added two examples for D7/D8. Also opened a follow-up; this change breaks the ability to create internal links during hook_install() (and potentially other situations where routes might not be available yet): #2348503: Drupal::l() can't be used for internal paths in hook_install() - Symfony throws "Route does not exist".

Status: Fixed » Closed (fixed)

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