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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
8.6 KB
  • Added url() to controller base and form base.
tim.plunkett’s picture

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.

dawehner’s picture

FileSize
749 bytes
8.6 KB

Just realized that array() is optional.

tim.plunkett’s picture

Title: Add a url generator helper to the form base/controllerbase » Add a UrlGenerator helper to FormBase and ControllerBase
tstoeckler’s picture

+++ 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.

pwolanin’s picture

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

dawehner’s picture

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

tstoeckler’s picture

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.)

alexpott’s picture

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.

pwolanin’s picture

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

dawehner’s picture

Redirect already works like that:

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

dawehner’s picture

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

pwolanin’s picture

Status: Needs review » Reviewed & tested by the community

Yes, this looks good.

alexpott’s picture

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
pwolanin’s picture

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?

pwolanin’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
9.35 KB

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

pwolanin’s picture

updated the issue summary a bit

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

pwolanin’s picture

Issue summary: View changes

Updated issue summary.

webchick’s picture

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().

tim.plunkett’s picture

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
9.74 KB
9.23 KB

how about this?

pwolanin’s picture

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you.

alexpott’s picture

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

Patch no longer applies.

twistor’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
9.29 KB
alexpott’s picture

Title: Add a UrlGenerator helper to FormBase and ControllerBase » Change 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!

catch’s picture

catch’s picture

dawehner’s picture

h3rj4n’s picture

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.

dawehner’s picture

Status: Reviewed & tested by the community » Fixed

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

xjm’s picture

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

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

xjm’s picture

Title: Change notice: Add a UrlGenerator helper to FormBase and ControllerBase » Add a UrlGenerator helper to FormBase and ControllerBase
xjm’s picture

Title: Add a UrlGenerator helper to FormBase and ControllerBase » Change 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.

xjm’s picture

**sigh**

xjm’s picture

Issue summary: View changes

Updated issue summary.

star-szr’s picture

Title: Change notice: Add a UrlGenerator helper to FormBase and ControllerBase » Add 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.