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
Issue summary updateWrite D8 patch.Review, commit.- Write D7 patch. (started but not up-to-date)
- 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)
Comment | File | Size | Author |
---|---|---|---|
#15 | drupal-7-documentation-2277623-4-update_drupal_parse_url_docs-2277623-15.patch | 2.85 KB | mparker17 |
Comments
Comment #1
jhodgdonYes, 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:
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.
Comment #2
mparker17I'll help!
Comment #3
mparker17Comment #4
mparker17Feedback welcome!
Comment #5
jhodgdonThanks! I reviewed the D8 patch... Looks mostly good. Some comments:
a)
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
say something like "See [link here] for an explanation of what the component parts are".
c)
There is no $path for purposes of this documentation. There is just the 'path' element of the return value array, right?
d)
Like the 'path' list element previous to this, 'query' and 'fragment' should not have '' around them here.
e)
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)
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?
Comment #6
mparker17Thanks 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.
Comment #7
mparker17Comment #8
jhodgdonGreat! One minor thing:
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.
Comment #9
mparker17@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.
Comment #10
mparker17Perhaps "violates the standard" is a strong way of wording it.
Essentially, this function:
Also note that PHP's parse_url() function doesn't follow the RFC to the letter either.
Comment #11
mparker17Okay, 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.Comment #12
jhodgdonExcellent! I agree with all of your changes.
Comment #13
jhodgdonThanks again! Committed to 8.x. Ready for D7 port.
Comment #14
mparker17Cool, I'll help!
Comment #15
mparker17I'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
Comment #16
mparker17Comment #17
jhodgdonLooks good, thanks!
Comment #19
jhodgdonComment #20
jhodgdon15: drupal-7-documentation-2277623-4-update_drupal_parse_url_docs-2277623-15.patch queued for re-testing.
Comment #22
jhodgdonThanks again! Committed to 7.x.