Followup from #2343759: Provide an API function to replace url()/l() for external urls. Part of #2339219: [meta] Finalize URL generation API (naming, docs, deprecation).
Following #2343759: Provide an API function to replace url()/l() for external urls, Url supports both routed and non-routed URLs. However, the constructor documentation and parameters as well as the class properties refer to route names even for unrouted URIs.
Clean this up.
There will be several @todo in the class referencing this issue.
None.
No public API changes.
#2343759: Provide an API function to replace url()/l() for external urls
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | 2346787-33.patch | 8.93 KB | ameymudras |
| #32 | 2346787-32.patch | 9.19 KB | nikhil_110 |
| #31 | 2346787-nr-bot.txt | 145 bytes | needs-review-queue-bot |
| #26 | 2346787-92x.patch | 8.98 KB | neclimdul |
Comments
Comment #1
tim.plunkettComment #2
webchickMoving to documentation component.
Comment #3
webchickComment #4
hussainwebGiving it a shot. I think I got all instances which need changing.
Comment #5
jhodgdon- An -> A
- What does this mean, anyway "URI without a Drupal route"? It just doesn't seem clear to me. Do you mean it's a URL that is outside the site? Or ... ? just not sure, and how do you distinguish the two?
Also there is a @todo in the constructor doc header that needs to be removed. ;)
Comment #6
hussainwebFixing as per #5.
@jhodgdon, "URI without a Drupal route" means that we are not using a route name. Do you have any suggestions on how we can phrase it better?
For the record, the phrase is used in several other places in the file and if it doesn't make sense, we should think about changing all the instances.
Comment #7
jhodgdonWell maybe it would be a bit clearer as:
The name of a route, or the full URI for URLs without routes (such as URLs outside the site).
Are there other reasons for having a URL without a route? Unsure...
I also don't think I like the term "unrouted", which is still in the latest patch in the member variable docs.
Comment #8
hussainwebSetting it to needs work. I will pick it up later today unless someone beats me to it.
Comment #9
xjmThanks @hussainweb! I think this is going to end up as a duplicate of #2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs and others, though.
Comment #10
xjmReopening per #2417459-38: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs point 3.
I think this is not just about documentation -- I think it is wrong, and weird, that we pass something called a route to the constructor, and then require something else to do
setUnrouted()to handle it as a URI, especially when we now have so many cases of that.Comment #11
xjmPostponing on #2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs which is all over this file.
Comment #12
xjmMy original summary even reflects that this is also about parameter and member names, not just docs, so moving back to routing as per the original issue that was filed.
Comment #13
mile23#2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs could use some review love.
Comment #14
dawehnerGiven that #2417459: Provide internal API for special schemes and thin public wrappers for user input and non-routed URLs is postponed for 8.1 we should move forward here
Comment #15
hussainwebSetting to needs work based on #8 and #9.
Comment #17
dawehnerI'm wondering whether we could do something like this
Comment #25
neclimdulSo I've been trying to unravel some bugs in base url handling and ran into this issue. I think Daniel's idea of splitting things out like that is a great idea! One of the hardest things I've run into is trying to figure out what the effect of any change to the Url class will be because it behaves different based on that internal weird internal state logic built from the external and unrouted properties. Classic state code smell.
I've rerolled this and I'm playing with what the logical outcomes of this might be. A RoutedUrl class, remove conditionals with routed and unrouted methods, traits, deprecations, etc. Maybe an interface?
Comment #26
neclimdulwoops, patch didn't make it on the comment.
Comment #31
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #32
nikhil_110 commentedAttached patch against Drupal 9.5.x
Comment #33
ameymudras commentedFixing the issues with the above patch
Comment #34
smustgrave commentedCan you please provide an interdiff with the patches.
Comment #37
avpadernoSince patches are no longer tested, a merge request should be provided.