Summary last updated: comment #11

Problem/Motivation

The documentation for D7's drupal_parse_url() and the corresponding function in D8, Drupal\Component\Utility\UrlHelper::parse() needs updating.

See comment in #1 for a list of what should be updated.

Proposed resolution

Update the docblock.

Remaining tasks

  1. Issue summary update
  2. Write D8 patch.
  3. Review, commit.
  4. Write D7 patch. (started but not up-to-date)
  5. Review, commit.

User interface changes

None.

API changes

None.

Original report by @regilero

API page: https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_...

I'm french, so I think I understand this text:

* 'query': An array of query parameters of $url, if existent.
* 'fragment': The fragment of $url, if existent.

But I'm pretty sur this is not regular english. maybe better with something like:
* 'query': An array of $url query parameters, if any (query string).
* 'fragment': $url fragment, if any (# tag)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Title: doc: drupal_parse_url, mix of french and english » UrlHelper::parse()/drupal_parse_url needs documentation update
Version: 7.x-dev » 8.x-dev
Issue tags: +Novice

Yes, this needs an update, thanks for reporting this!

Actually this whole documentation block could use a bit of help...

a) The information about the return value should be part of the @return documentation, not separate... or maybe it should just be dropped. This part:

The returned array contains a 'path' that may be passed separately to url(). For example:

$options = drupal_parse_url($_GET['destination']);
$my_url = url($options['path'], $options);
$my_link = l('Example link', $options['path'], $options);

This is required, because url() does not support relative URLs containing a query string or fragment in its $path argument. Instead, any query string needs to be parsed into an associative query parameter array in $options['query'] and the fragment into $options['fragment'].

It is really not telling us much useful about this function. If it needs to be documented, it should be on url() not here.

b) In the $url parameter: " f.e. $_GET['destination']." --> f.e. should be written out as "for example, " and it needs a ; before it.

c) In the return values, "if existent" should be "if any".

d) Hm... Actually the summary of this function says that it should not be used for external URLs, but then the return value says it can be. So, the part in the summary that says not to use it for external URLs should be removed. The code definitely supports external URLs.

So, this is probably a good Novice project...

It needs to be done in Drupal 8 first on Drupal\Component\Utility\UrlHelper::parse(), and then backported to Drupal 7 on the drupal_parse_url() function.

mparker17’s picture

Assigned: Unassigned » mparker17

I'll help!

mparker17’s picture

Issue summary: View changes
mparker17’s picture

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! I reviewed the D8 patch... Looks mostly good. Some comments:

a)

+   * Parses a URL string into it's path, query and fragment components.

1. it's ==> its
2. We use serial commas in Drupal, so "... path, query, and fragment components" (this applies elsewhere in the patch too).

b) In general, we do not document standard PHP or terminology. So I don't think we need to explain what query, fragment, etc. are. So I would suggest taking out that paragraph, and maybe instead at the end of

+   * This function splits both internal paths like @code node?b=c#d @endcode and
+   * external URLs like @code https://example.com/a?b=c#d @endcode into their
+   * component parts.

say something like "See [link here] for an explanation of what the component parts are".

c)

+   * Note, however, that when passed an external URL, this function groups the
+   * scheme, authority and path together as $path.

There is no $path for purposes of this documentation. There is just the 'path' element of the return value array, right?

d)

+   *   - 'query': An array of query parameters from $url, if they exist.
+   *   - 'fragment': The fragment component from $url, if it exists.

Like the 'path' list element previous to this, 'query' and 'fragment' should not have '' around them here.

e)

+   *   Note that the returned array contains a 'path' that must be passed
+   *   separately to url() and l(). For example:
+   *   @code
    *   $options = UrlHelper::parse(\Drupal::request()->query->get('destination'));
    *   $my_url = url($options['path'], $options);
    *   $my_link = l('Example link', $options['path'], $options);
-   * @endcode
+   *   @endcode

I think we can leave this out. We do not need to explain on this function how to call url() -- that should be documented on url().

f)

+   * @see url()

url() is deprecated in 8.x, so we should probably not @see link to it.... hm... well it is not exactly deprecated, but it has zero documentation on its parameters.... I wonder what happened there? Separate issue, but we should probably do @see \Drupal::ur() instead?

Other issue for url() problems: #2278559: url() function docs have vanished?

mparker17’s picture

Thanks for the great review!

a: Fixed and fixed. That possessive-apostrophe-exception gets me every time >_<
b: Fixed.
c: Re-worded slightly to make it more clear.
d: Whoops! Fixed
e: I agree. Removed.
f: Since the function still exists, it's probably worth keeping the reference. Changed to @see \Drupal::url().

Feedback welcome!


Note that I have not updated the D7 patch with this feedback yet. Going to wait until the D8 patch gets in before continuing work on that.

mparker17’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Great! One minor thing:

+   * Note, however, that when passed an external URL, this function groups the
+   * scheme, authority and path together into the path component.

This is another list (needs serial comma before "and" in the 2nd line). Also I think it would be better off as part of the previous paragraph? Or maybe it could be omitted, because the @return says this?

Also the @return says it groups "scheme and host" with path, and this says "scheme, authority". Which is correct?

Oh, and you shouldn't listen to me... Drupal::url() doesn't actually take $path as an argument, but $route, so it is not relevant here... Not sure what we should link to really... maybe just leave it as url() and hope that other issue gets fixed? Sigh.

mparker17’s picture

@jhodgdon whoops: that list one is going to get me every time too *sighs*.

I wanted to note that the function groups the scheme, authority and path together prominently because it's a Drupalism: it violates the RFC standard it's based on.

The omission of "authority" from the @return statement is my error.

mparker17’s picture

Perhaps "violates the standard" is a strong way of wording it.

Essentially, this function:

  • Doesn't output the scheme or authority sections on their own.
  • Overloads the term "path" to contain more than what the RFC considers is the "path".
  • Includes special code to work around the fact that the parse_url() function doesn't support relative paths
  • Includes special code to work around the fact that the parse_url() function tries to split URLs the way that the RFC specifies.

Also note that PHP's parse_url() function doesn't follow the RFC to the letter either.

mparker17’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.75 KB
1.46 KB

Okay, here's a new patch to fix the serial commas and inconsistent return statement wording.

I've changed the second description paragraph to explicitly state when this function doesn't follow the RFC.

I checked the url() vs \Drupal::url() functions: both exist, but the first one is called as url($path [, $options]), so I've changed the @see statement to the way it was in #4.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Excellent! I agree with all of your changes.

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

Thanks again! Committed to 8.x. Ready for D7 port.

mparker17’s picture

Assigned: Unassigned » mparker17

Cool, I'll help!

mparker17’s picture

I've double-checked that the Docblock for D8 still applies to D7: it does (although the D7 one should reference drupal_goto()).

To keep things simple, I'm pretending that I didn't write a patch for D7 in #4... :P

mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Patch (to be ported) » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

Status: Reviewed & tested by the community » Needs work
jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community
jhodgdon’s picture

  • Commit 921d9dd on 7.x by jhodgdon:
    Issue #2277623 by mparker17: Fix up docs for drupal_parse_url function
    
jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 7.x.

  • Commit 1dee6b7 on 8.x by jhodgdon:
    Issue #2277623 by mparker17: Update url parse documentation
    

Status: Fixed » Closed (fixed)

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