Updated: Comment #17

Problem/Motivation

It should be as easy as possible to have something like url() but using route names and parameters on controllers.

Proposed resolution

Add a url() method to FormBase adn ControllerBase so devs can call $this->url() without needing to use the DIC

Remaining tasks

None

User interface changes

None

API changes

No breaking changes.
New url() method added to 2 classes.

#2078285: Add short-cut methods to the \Drupal class for generating URLs and links from routes
#2073811: Add a url generator twig extension
#2078287: [meta] Fix the DX of the link generator

Files: 
CommentFileSizeAuthor
#24 url-2073813-24.patch9.29 KBtwistor
PASSED: [[SimpleTest]]: [MySQL] 58,337 pass(es).
[ View ]
#21 url-2073813-21.patch9.25 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,470 pass(es).
[ View ]
#21 2073813-20-21.increment.txt2.04 KBpwolanin
#20 url-2073813-20.patch9.23 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#20 2073813-16-20.increment.txt9.74 KBpwolanin
#16 url-2073813-16.patch9.35 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,009 pass(es).
[ View ]
#8 2073813-8-controller-form-base-url.patch9.65 KBtstoeckler
PASSED: [[SimpleTest]]: [MySQL] 58,489 pass(es).
[ View ]
#8 interdiff-2073813-7-8.txt1.27 KBtstoeckler
#8 shortcut-add-inline-broken.png48.4 KBtstoeckler
#7 url_generator-2073813-7.patch9.34 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,509 pass(es).
[ View ]
#7 interdiff.txt4.1 KBdawehner
#6 url-2073813-4-6.increment.txt2.38 KBpwolanin
#6 url-2073813-6.patch8.63 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 58,259 pass(es).
[ View ]
#3 url-2073813-4.patch8.6 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,418 pass(es).
[ View ]
#3 interdiff.txt749 bytesdawehner
#1 url-2073813-1.patch8.6 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,119 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new8.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,119 pass(es).
[ View ]
  • Added url() to controller base and form base.

Status:Needs review» Reviewed & tested by the community

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php
@@ -50,7 +50,7 @@ public function addShortcutLinkInline(ShortcutSetInterface $shortcut_set, Reques
-      return new RedirectResponse($this->urlGenerator()->generateFromPath('<front>', array('absolute' => TRUE)));
+      return $this->redirect('front', array());

Whoo! This is nice.

I don't think we need further test coverage here, we either have the coverage for the form or we don't, and the API we're using has coverage.
The docs are excellent, especially all of the @throws part, that's helpful.

StatusFileSize
new749 bytes
new8.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,418 pass(es).
[ View ]

Just realized that array() is optional.

Title:Add a url generator helper to the form base/controllerbaseAdd a UrlGenerator helper to FormBase and ControllerBase

+++ b/core/lib/Drupal/Core/Controller/ControllerBase.php
@@ -180,4 +180,49 @@ public function redirect($route_name, array $parameters = array(), $status = 302
+   * @param string $name
+   *   The name of the route

Can we name this $route_name in FormBase and ControllerBase? In UrlGenerator::generatorFromRoute() it's clear what $name means, but here it would be useful to be more explicit IMO.

StatusFileSize
new8.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,259 pass(es).
[ View ]
new2.38 KB

Makes sense to me - also makes it consistent with the redirect() method. Just changing the variable names, so leaving at rtbc.

StatusFileSize
new4.1 KB
new9.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,509 pass(es).
[ View ]

At least in the link generator we also use #route_parameters.

StatusFileSize
new48.4 KB
new1.27 KB
new9.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,489 pass(es).
[ View ]

Noticed some strange whitespace, so quickly re-rolled for that. Leaving RTBC.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php
@@ -50,7 +50,7 @@ public function addShortcutLinkInline(ShortcutSetInterface $shortcut_set, Reques
-      return $this->redirect('front');
+      return $this->redirect('<front>');

This actually indicates that we have missing test coverage for adding a shortcut inline. Opened #2075557: Missing test coverage for adding a shortcut link inline for that. (See also the attached screenshot.)

Status:Reviewed & tested by the community» Needs review

So we've had 4 patches since the rtbc in #2. Setting back to needs review so we can ensure that the current patch should be rtbc.

As a follow-up we should add another redirect() method (?) that takes a route and params?

Redirect already works like that:

public function redirect($route_name, array $parameters = array(), $status = 302);

I would be fine to set this as RTBC ... ;)

Status:Needs review» Reviewed & tested by the community

Yes, this looks good.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

git ac https://drupal.org/files/2073813-8-controller-form-base-url.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  9885  100  9885    0     0  10179      0 --:--:-- --:--:-- --:--:-- 12481
error: patch failed: core/lib/Drupal/Core/Form/FormBase.php:8
error: core/lib/Drupal/Core/Form/FormBase.php: patch does not apply

Added Drupal:url() back to #2078285: Add short-cut methods to the \Drupal class for generating URLs and links from routes, since it's not in the patch and seems better suited there?

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new9.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,009 pass(es).
[ View ]

trivial conflict in the use statements due to the patch that added renamed to Drupal\Core\DependencyInjection\ContainerInjectionInterface;

updated the issue summary a bit

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Status:Reviewed & tested by the community» Needs work

Let's not add all of those docs for url() multiple times, but rather put a @see in there, similar to how we do t().

Also while rerolling, be sure to say URL in comments, not url (when its not the class name)

Status:Needs work» Needs review
StatusFileSize
new9.74 KB
new9.23 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

how about this?

StatusFileSize
new2.04 KB
new9.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,470 pass(es).
[ View ]

dawehner points out that the docs are actually on the interfaces.

Status:Needs review» Reviewed & tested by the community

Thank you.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new9.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,337 pass(es).
[ View ]

Title:Add a UrlGenerator helper to FormBase and ControllerBaseChange notice: Add a UrlGenerator helper to FormBase and ControllerBase
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed 95c2e17 and pushed to 8.x. Thanks!

Status:Needs review» Reviewed & tested by the community

I think the text on the pages is sufficient. Example code isn't needed for these functions as long as there is at least one occurrence in core as reference I guess.

Status:Reviewed & tested by the community» Fixed

Thank you, ... let's mark it was fixed.

Priority:Major» Normal
Issue tags:-Needs change record

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

Title:Change notice: Add a UrlGenerator helper to FormBase and ControllerBaseAdd a UrlGenerator helper to FormBase and ControllerBase

Title:Add a UrlGenerator helper to FormBase and ControllerBaseChange notice: Add a UrlGenerator helper to FormBase and ControllerBase
Priority:Normal» Major
Status:Fixed» Active
Issue tags:+Needs change record

d.o--

So we need to reference this issue on those two change notices.

**sigh**

Issue summary:View changes

Updated issue summary.

Title:Change notice: Add a UrlGenerator helper to FormBase and ControllerBaseAdd a UrlGenerator helper to FormBase and ControllerBase
Priority:Major» Normal
Issue summary:View changes
Status:Active» Fixed
Issue tags:-Needs change record

I added this issue to the two change notices, if that's all that's needed on top of @dawehner's edits this issue can be closed now.

https://drupal.org/node/2060189
https://drupal.org/node/2076011

Status:Fixed» Closed (fixed)

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