Problem/Motivation

Subport of #2339219-55: [meta] Finalize URL generation API (naming, docs, deprecation)

Proposed resolution

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#11 2346361-11.patch12.44 KBpwolanin
#11 increment.txt753 bytespwolanin
#7 2346361-7.patch12.44 KBpwolanin
#4 interdiff.txt2.1 KBdawehner
#4 2346361-4.patch17.38 KBdawehner
#1 2346361-1.patch16.82 KBdawehner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Active » Needs review
FileSize
2346361-1.patch16.82 KB

First bit, conversions are done in the repo at the same time.

Crell’s picture

Crell CreditAttribution: Crell commented 
  1. +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -0,0 +1,149 @@
+    if (isset($options['https'])) {

    Set https to false by default in addOptionsDefault() and this check can go away. 

  2. +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -0,0 +1,149 @@
+      $uri .= (strpos($uri, '?') !== FALSE ? '&' : '?') . UrlHelper::buildQuery($options['query']);

    buildQuery() is documented as unneeded as of PHP 5.4, which is now a required version anyway. Per its docblock we can just use http_build_query() directly and eliminate the dependency. 

  3. +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -0,0 +1,149 @@
+  protected function addOptionDefaults(array &$options) {

    Should this method also enforce legal values? Eg, that absolute and https can only be booleans and nothing else? 

  4. +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssemblerInterface.php
@@ -0,0 +1,54 @@
+   * This is a helper function that calls buildExternalUrl() or buildLocalUrl()
+   * based on a check of whether the path is a valid external URL.

    Since those methods are internal to one particular implementation this comment is inappropriate on the interface.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

If you do #2.2 you will need to use the PHP_QUERY_RFC3986 constant.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
FileSize
2346361-4.patch17.38 KB
interdiff.txt2.1 KB

Set https to false by default in addOptionsDefault() and this check can go away.

Actually not because we handle three cases:

  • Not change the schema (NULL)
  • Change the schema from https to http (FALSE)
  • Change the schema from http to https (TRUE)

Should this method also enforce legal values? Eg, that absolute and https can only be booleans and nothing else?

Well, if people add new (non-existing) options and they are not doing anything I am not convinced that it is worth the effort
both in term of more complexity in the code and performance.

If you do #2.2 you will need to use the PHP_QUERY_RFC3986 constant.

Originally I though this would be a great idea, though yeah this is just another level of discussion (do we still want to help the old wrapper etc.) Let's do that in another place, sorry.

The last submitted patch, 1: 2346361-1.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2346361-4.patch, failed testing.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Status: Needs work » Needs review
FileSize
2346361-7.patch12.44 KB

Looks like dawehner included changes from the next part of the patch. Let me try to roll this again.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
Issue tags: +Amsterdam2014
pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented

re: UrlHelper::buildQuery(), it's not that simple - I have a patch that made it a wrapper on the built-in and it does somewhat different encoding than Drupal core does, so making such a change in this patch would likely generate pointless test fails all over.

see: #2248257: Optimize UrlHelper::buildQuery() to use http_build_query()

Crell’s picture

Crell CreditAttribution: Crell commented 
+++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssemblerInterface.php
@@ -14,8 +14,8 @@
+   * For actual implementations the logic probably has to be splitted up

"split up", not splitted.

Otherwise, if we're not fixing UrlHelper here then this seems fine.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin commented
FileSize
increment.txt753 bytes
2346361-11.patch12.44 KB

ok, fixed that code comment.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Cool

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented 
  1. +++ b/core/lib/Drupal/Core/Utility/UnroutedUrlAssembler.php
@@ -0,0 +1,152 @@
+ * Provides a way to build external or non Drupal local domain URLs.

    Durpal :) cheeky. 

  2. +++ b/core/tests/Drupal/Tests/Core/Utility/UnroutedUrlAssemblerTest.php
@@ -0,0 +1,129 @@
+  public function testAssembleWithNeitherExternalNokDomainLocalUri() {

    Nok? I think this is Nor

I think both of these should be fixed on commit. I'm +1 for rtbc.

webchick’s picture

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

Fixed Alex's nits and committed and pushed to 8.x. Thanks!

  • webchick committed da1b664 on 8.0.x 
    Issue #2346361 by pwolanin, dawehner: Add a UnroutedUrlAssembler and put...

Status: Fixed » Needs work

The last submitted patch, 11: 2346361-11.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Needs work » Fixed

Silly testbot, trix are for kids.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Parent issue: » #2339219: [meta] Finalize URL generation API (naming, docs, deprecation)

Status: Fixed » Closed (fixed)

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