ConfirmFormBase::getCancelPath() should allow for a route

Tagging.

cross post.

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

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

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.

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

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

The amount of duplicated code here is depressing.

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

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

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

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.

Thank you

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.

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.

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.

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

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

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

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

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

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.

+++ 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');

Oh, that is lovely.

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.

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

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:

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.

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.

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.

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.

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

This is a trivial change so back to RTBC.

All right then! :)

Committed and pushed to 8.x. Thanks!

Will need a change notification on this.

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.