Updated: Comment #25

Problem/Motivation

ConfirmFormBase has a getCancelPath() method, which points to what is usually some kind of parent for the form. This requires a URL string pointing to the router path - i.e. admin/config/foobar

However ConfirmFormBase itself is designed to be used as a route controller - and those, by design are path independent (as will their 'parent' be when those are converted. So really I'd expect this method to provide arguments to pass to the generator as opposed to an actual path - then if the parent gets moved somewhere, that'll work too.

Proposed resolution

Use the new route capabilities of '#type' => 'link', and introduce a getCancelRoute() method to replace getCancelPath().

Remaining tasks

Get blockers in

User interface changes

N/A

API changes

ConfirmFormInterface::getCancelPath() is deprecated and slated for removal pre-8.0

Files: 
CommentFileSizeAuthor
#35 drupal8.forms-system.1963394-35.patch47.56 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 59,359 pass(es).
[ View ]
#35 interdiff.txt772 bytesjibran
#31 confirm-routes-1963394-31.patch47.57 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es).
[ View ]
#29 interdiff.txt813 bytestim.plunkett
#29 confirm-routes-1963394-29.patch48.55 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,770 pass(es).
[ View ]
#28 confirm-routes-1963394-28.patch48.58 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,727 pass(es).
[ View ]
#28 interdiff.txt16.54 KBtim.plunkett
#25 confirm-routes-1963394-25.patch45.24 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,894 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#25 interdiff.txt3.41 KBtim.plunkett
#24 confirm-routes-1963394-24.patch45.99 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,424 pass(es).
[ View ]
#24 interdiff.txt2.1 KBtim.plunkett
#22 confirm-routes-1963394-22.patch43.92 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 726 fail(s), and 43 exception(s).
[ View ]
#22 interdiff.txt35.91 KBtim.plunkett
#15 confirm-form-1963394-15.patch14.23 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,001 pass(es).
[ View ]
#15 interdiff.txt1.76 KBtim.plunkett
#13 confirm-form-1963394-13.patch14.06 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,511 pass(es).
[ View ]
#13 interdiff.txt12.93 KBtim.plunkett
#11 confirm-cancel-1963394-11.patch9.6 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,426 pass(es).
[ View ]
#11 interdiff.txt8.59 KBtim.plunkett
#8 confirm-route-1963394-8.patch6.02 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,472 pass(es).
[ View ]

Comments

catch’s picture

Title:ConfirmFormBase requires a hard-coded path» ConfirmFormBase::getCancelUrl() should allow for a route
tim.plunkett’s picture

Title:ConfirmFormBase::getCancelUrl() should allow for a route» ConfirmFormBase::getCancelPath() should allow for a route
Issue tags:+FormInterface
jibran’s picture

Title:ConfirmFormBase::getCancelPath() should allow for a route» ConfirmFormBase requires a hard-coded path
Issue tags:+WSCCI-conversion

Tagging.

jibran’s picture

Title:ConfirmFormBase requires a hard-coded path» ConfirmFormBase::getCancelPath() should allow for a route

cross post.

Crell’s picture

Oh drat. That's completely right, but will be annoying to change now that conversions have started.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett
Status:Active» Postponed

Since this is used for #type => link, this is blocked on #2047619: Add a link generator service for route-based links

effulgentsia’s picture

Status:Postponed» Active
tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new6.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,472 pass(es).
[ View ]

This defers to a route name if present, but leaves in paths for now (not all of the paths used as the cancel link are converted yet).

I've converted two for now, one regular and one with parameters.

Oh how I long for traits...

Also, #2073813: Add a UrlGenerator helper to FormBase and ControllerBase will let us remove the \Drupal::urlGenerator() calls.

dawehner’s picture

I would like to see a @deprecated on getCancelPath and suggest people to use getCancelRoute

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Form/ConfirmFormBase.php
    @@ -46,7 +46,17 @@ public function getFormName() {
    +      $path = \Drupal::urlGenerator()->generateFromRoute($route['name'], $route['parameters'], $route['options']);
    ...
    +      $path = $this->getCancelPath();

    +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Form/VocabularyDeleteForm.php
    @@ -32,8 +32,10 @@ public function getQuestion() {
    -    return 'admin/structure/taxonomy';

    Note: the url generator generates a path with "/" at the front, so this won't work. Instead of using the url generator here you should better use the new functionality of #type link to pass in route_name and route_parameters

  2. +++ b/core/lib/Drupal/Core/Form/ConfirmFormInterface.php
    @@ -34,6 +34,20 @@ public function getQuestion();
    +   *     \Drupal\Core\Routing\UrlGeneratorInterface::generateFromPath() for

    Better point to generateFromRoute

tim.plunkett’s picture

StatusFileSize
new8.59 KB
new9.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,426 pass(es).
[ View ]

The amount of duplicated code here is depressing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityConfirmFormBase.php
@@ -85,25 +85,63 @@ protected function actions(array $form, array &$form_state) {
     // Prepare cancel link.
     $query = $this->getRequest()->query;
+    // If a destination is specified, that serves as the cancel link.
     if ($query->has('destination')) {
       $options = Url::parse($query->get('destination'));
+      $link = array(
+        '#href' => $options['path'],
+        '#options' => $options,
+      );
     }
-    elseif (is_array($path)) {
-      $options = $path;
+    // Check for a route-based cancel link.
+    elseif (($route = $this->getCancelRoute()) && !empty($route['name'])) {
+      // Ensure there is something to pass as the params and options.
+      $route += array(
+        'parameters' => array(),
+        'options' => array(),
+      );
+      $link = array(
+        '#route_name' => $route['name'],
+        '#route_parameters' => $route['parameters'],
+        '#options' => $route['options'],
+      );
     }
+    // Check for a path-based cancel link.
+    // @todo Remove when all forms use route-based cancel links.
     else {
-      $options = array('path' => $path);
+      $path = $this->getCancelPath();
+      if (is_array($path)) {
+        $link = array(
+          '#href' => $path['path'],
+          '#options' => $path,
+        );
+      }
+      else {
+        $link = array(
+          '#href' => $path,
+        );
+      }
     }

What blocks us from having a helper object which does just that? Composition/reuse of existing code without inheritance is not bad.

tim.plunkett’s picture

StatusFileSize
new12.93 KB
new14.06 KB
PASSED: [[SimpleTest]]: [MySQL] 58,511 pass(es).
[ View ]

Very good point!
Also wrote a unit test for the helper.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Form/ConfirmFormHelperTest.php
@@ -0,0 +1,138 @@
+ *
+ * @group Form
+ */
+class ConfirmFormHelperTest extends UnitTestCase {

Let's also one @group Drupal, so you can run just them.

In general I am not sure about the getCancelRoute method name, but getCancelRouteInformation sounds odd.

tim.plunkett’s picture

StatusFileSize
new1.76 KB
new14.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,001 pass(es).
[ View ]

I think in the context of this class, getCancelRouteInformation() is overly verbose. There is no confusion about whether you should return a Route object or not, I think the current name is fine.

Added the @group, and removed some obsolete use statements.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you

webchick’s picture

Status:Reviewed & tested by the community» Needs review

Ick. I don't think it makes sense to have both of these methods. We should pick one and use it everywhere. If anything needs to use the path, it should be converted in this patch IMO.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

The problem is that we don't have routes everywhere yet, so what about marking getCancelPath as depricated as recommend to use getCancelRoute if possible ...
Oh see the patch already does that.

Once everything is converted we can remove it.

webchick’s picture

Status:Reviewed & tested by the community» Needs work

My concern is specifically around these "base" classes defining both. That's the primary way that module developers are going to grasp the D8 APIs. Yes, if you use a sophisticated IDE like PHPStorm, {@inheritdoc} will fill in the blanks, but if you don't, it won't. It looks like you need to implement both methods, since both are public.

Tim and I discussed marking them more explicitly deprecated in the base classes, and that would address my concern.

dawehner’s picture

Does that mean that you suggest to even keep this functionality for a long time? It feels wrong to keep getCancelPath() until the release.

webchick’s picture

No, I want to see it removed before release. Which... turns this normal bug into a critical task which... again I'm wondering if it's better to just postpone it on route conversions. :(

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new35.91 KB
new43.92 KB
FAILED: [[SimpleTest]]: [MySQL] 57,564 pass(es), 726 fail(s), and 43 exception(s).
[ View ]

There are 9 blockers:
(EDIT: moved to the issue summary)

I'd much rather get this in, and fix it in those 9 issues...

Status:Needs review» Needs work

The last submitted patch, confirm-routes-1963394-22.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new2.1 KB
new45.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,424 pass(es).
[ View ]

This is your brain. This is your brain on Durpal.

tim.plunkett’s picture

StatusFileSize
new3.41 KB
new45.24 KB
FAILED: [[SimpleTest]]: [MySQL] 58,894 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

7 conversions remain.

Found a fun bug in assertUrl() in the process.
It turns out that neither Drupal nor any web browser care about the difference between these two:

/admin?destination=admin/config
/admin?destination=admin%2Fconfig

But simpletest did care.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
@@ -0,0 +1,83 @@
+    // Check for a path-based cancel link.
+    // @todo Remove when all forms use route-based cancel links.
+    else {
+      $path = $form->getCancelPath();
+      if (is_array($path)) {
+        $link = array(
+          '#href' => $path['path'],
+          '#options' => $path,
+        );
+      }
+      else {
+        $link = array(
+          '#href' => $path,
+        );
+      }
+    }

To address #19, what if we remove getCancelPath() from the interface, and remove the above code block. Then, for each form that still needs to use a path, it can override buildForm(), call parent::buildForm(), then do something like:

// @todo Remove when this path is converted to a route: [#ISSUE_FOR_THE_ROUTE_CONVERSION]
$form['actions']['cancel'] += array('#href' => 'some/legacy/path');
webchick’s picture

Oh, that is lovely.

tim.plunkett’s picture

StatusFileSize
new16.54 KB
new48.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,727 pass(es).
[ View ]

Brilliant idea, thanks @effulgentsia.

I think #2073813: Add a UrlGenerator helper to FormBase and ControllerBase is about to go in (and I had to include a Drupal:: call with a @todo for that), but I wanted to post this before I went to lunch to see what the bot thinks.

tim.plunkett’s picture

StatusFileSize
new48.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,770 pass(es).
[ View ]
new813 bytes

Yay, that went in. We should be done here now.

dawehner’s picture

One general question: this pattern will potentially appear a lot, so returning route name, parameters and potentially options for the link,
so do we really want to force people to specify 'route_name', route_parameters all the time and not use something simpler like:

<?php
return array('name', array('key' => 'value'), array('fragment' => 'bar'));
?>
  1. +++ b/core/modules/comment/lib/Drupal/comment/Form/ConfirmDeleteMultiple.php
    @@ -112,7 +111,11 @@ public function buildForm(array $form, array &$form_state, Request $request = NU
    +    // @todo Convert to getCancelRoute() after http://drupal.org/node/1986606.
    +    $form['actions']['cancel']['#href'] = 'admin/content/comment';

    Even then you still have a fallback ...

  2. +++ b/core/modules/comment/lib/Drupal/comment/Form/DeleteForm.php
    @@ -26,8 +26,19 @@ public function getQuestion() {
    +    $actions['cancel']['#href'] = 'node/' . $this->entity->nid->target_id;

    Should we just use $this->entity->uri() instead? But yeah this is temporary so I don't care.

tim.plunkett’s picture

StatusFileSize
new47.57 KB
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es).
[ View ]

I debated skipping the key names, but it just seemed messier and more confusing.

For both 1+2, I'd rather just stick with the path as-is until the conversion happens.

Straight reroll.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

I think it is ready to fly.

+++ b/core/modules/path/lib/Drupal/path/Form/DeleteForm.php
@@ -77,7 +76,11 @@ public function getCancelPath() {
+    $form = parent::buildForm($form, $form_state);
+
+    // @todo Convert to getCancelRoute() after http://drupal.org/node/1987802.
+    $form['actions']['cancel']['#href'] = 'admin/config/search/path';
+    return $form;

TBH I don't like this but I am happy that we are removing a WTF situation of keeping geCancelPath.

webchick’s picture

Awesome work!

Going to leave this at RTBC for a couple of days for comment, esp. from other WSCCI-ans and/or core maintainers, but IMO this looks great, and doesn't leave us in an unstable state regarding D8's APIs.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/system/lib/Drupal/system/Form/ModulesListConfirmForm.php
@@ -107,7 +109,7 @@ public function buildForm(array $form, array &$form_state) {
+      return new RedirectResponse($this->getUrlGenerator()->generate('system_modules_list', array(), TRUE));

#2077513: Refactor ControllerBase to be more consistent with FormBase has changed getUrlGenerator to urlGenerator

jibran’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new772 bytes
new47.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,359 pass(es).
[ View ]

This is a trivial change so back to RTBC.

webchick’s picture

Title:ConfirmFormBase::getCancelPath() should allow for a route» Change notice: ConfirmFormBase::getCancelPath() should allow for a route
Category:bug» task
Priority:Normal» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

All right then! :)

Committed and pushed to 8.x. Thanks!

Will need a change notification on this.

tim.plunkett’s picture

Title:Change notice: ConfirmFormBase::getCancelPath() should allow for a route» ConfirmFormBase::getCancelPath() should allow for a route
Status:Active» Fixed
Issue tags:-Needs change record

I updated https://drupal.org/node/1945416 which has the details of the conversion, but also opened https://drupal.org/node/2089281 to notify people of the change.

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

Anonymous’s picture

Issue summary:View changes

updated