Split ShortcutForm into ShortcutAddForm and ShortcutEditForm

Working on this @ DUG Belgium

Created patch. I was unable to test it because of the following error when visiting url : admin/config/user-interface/shortcut

Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Route "field_ui.overview_shortcut" does not exist.") in "core/modules/system/templates/links.html.twig" at line 50. in Twig_Template->displayWithErrorHandling() (line 291 of core/vendor/twig/twig/lib/Twig/Template.php).

This should need a reroll after #2428003: Move all shortcut forms to \Drupal\shortcut\Form.

Split the ShortcutForm.php to ShortcutAddForm.php and ShortcutEditForm.php. Made the changes in the annotations in Entity folder files to reflect the changes.

Update to run the test bot

This issue is dependant on the application of patch in - #2428003: Move all shortcut forms to \Drupal\shortcut\Form . Should we wait till that patch is applied?

You can combine the patches for now to get the proper test results.

1. +++ b/core/modules/shortcut/shortcut.routing.yml
   @@ -57,7 +57,7 @@ shortcut.link_add:
    entity.shortcut.canonical:
      path: '/admin/config/user-interface/shortcut/link/{shortcut}'
   
   @@ -66,7 +66,7 @@ entity.shortcut.canonical:
    entity.shortcut.edit_form:
      path: '/admin/config/user-interface/shortcut/link/{shortcut}'
   

   Probably out of scope, but there shouldn't two of these

2. +++ b/core/modules/shortcut/src/Entity/Shortcut.php
   @@ -26,9 +26,9 @@
   - *       "default" = "Drupal\shortcut\ShortcutForm",
   - *       "add" = "Drupal\shortcut\ShortcutForm",
   ...
   + *       "default" = "Drupal\shortcut\Form\ShortcutAddForm",
   + *       "add" = "Drupal\shortcut\Form\ShortcutAddForm",
   

   Can we please remove this form operation?

3. +++ b/core/modules/shortcut/src/Entity/ShortcutSet.php
   @@ -22,9 +22,9 @@
   - *       "default" = "Drupal\shortcut\ShortcutSetForm",
   - *       "add" = "Drupal\shortcut\ShortcutSetForm",
   - *       "edit" = "Drupal\shortcut\ShortcutSetForm",
   + *       "default" = "Drupal\shortcut\Form\ShortcutSetForm",
   + *       "add" = "Drupal\shortcut\Form\ShortcutSetForm",
   + *       "edit" = "Drupal\shortcut\Form\ShortcutSetForm",
   

   This should let #2324877: Split ShortcutSetForm into ShortcutSetAddForm and ShortcutSetEditForm handle ShortcutSet and not move it

4. +++ /dev/null
   @@ -1,46 +0,0 @@
   -    if ($status == SAVED_UPDATED) {
   -      $message = $this->t('The shortcut %link has been updated.', array('%link' => $entity->getTitle()));
   -    }
   -    else {
   -      $message = $this->t('Added a shortcut for %title.', array('%title' => $entity->getTitle()));
   -    }
   

   Honestly, this method is so simple I have no problem leaving it and won't fixing the issue.

There isn't sufficient motivation for this change in the issue summary.

I don't quite see how this patch is dependent on #2428003: Move all shortcut forms to \Drupal\shortcut\Form
The tests must be failing because of some other reason.

I think we should set default form to edit the we wouldn't need to edit the routing.yml

@tim.plunkett
I don't quite understand what is mention in #11 4th comment.
Do you mean we should keep the same save() method in new ShortcutAddForm and ShortcutEditForm classes? ( This makes no sense though )

#11 1, Should we open a new issue for this?

New patch attached.

Is this good enough summary update for motivation?

We should not remove the ShortcutForm class here.
Patch fix.

Go test bot!

ShortcutForm uses the old pattern of checking whether it is an add or an update form to provide dedicated messages.

Who decided that it was an "old pattern", and that a newer pattern would be better?
I think this class is fine as is.

Moving it to another namespace is fine.

Yes this does lead to redundant code.
The motivation for this might have been the fact that deleteform has a separate class but edit and add don't.

Though only changing the namespace seems fine and this should be closed as works as desgined and let the #2428003: Move all shortcut forms to \Drupal\shortcut\Form handle this.

Views?

This is now RTBC #2428003: Move all shortcut forms to \Drupal\shortcut\Form
Rerolled this patch

Updating issue summary.

Thanks for rerolling the patch @naveenvalecha.

1. +++ b/core/modules/shortcut/src/Form/ShortcutAddForm.php
   @@ -0,0 +1,33 @@
   + * Form handler for the shortcut Add entity forms.
   

   s/Add/add.

2. +++ b/core/modules/shortcut/src/Form/ShortcutEditForm.php
   @@ -0,0 +1,33 @@
   + * Form handler for the shortcut Edit entity forms.
   

   s/Edit/edit.

3. +++ /dev/null
   @@ -1,51 +0,0 @@
   -      $view_link = \Drupal::l($entity->getTitle(), $url);
   

   What about this?

Thank you @Jibran, Updated patch file as per #23. Entity title link based on access added back.

Thanks for fixing the feedback. I think we can remove

  /**
   * The entity being used by this form.
   *
   * @var \Drupal\shortcut\ShortcutInterface
   */
  protected $entity;

from both classes.

Thank you @Jibran, Updated patch as per #25.

Thanks for the quick update. Can you please use git diff -M20 to create a patch?

Thanks @Jibran.
Here is the diff file between #20 and #26

I was asking about updating the patch like this. Anyways I rerolled the patch. Thank you for your contribution. I think this patch is RTBC now.

Can someone explain how this falls into https://www.drupal.org/core/d8-allowed-changes? I'm not sure the current change is. But I might be wrong.

I think we can add the new forms but I'm not sure we can remove the old form - we need to deprecate it.

Please have a look at #2428003-27: Move all shortcut forms to \Drupal\shortcut\Form.

RTBC +1
Re: #30
Per https://www.drupal.org/core/d8-bc-policy#controllers The Form classes are internals

@alexpott the form classes are entirely @internal so it should be fine.

We could add back the old class with a deprecation notice and not use it, but for me the likelihood of someone extending shortcut forms should be close to zero?

I think we need a change record here. The CR needs to explicit mention how a contrib module could support both 8.2.x and 8.3.x if they are extending these classes. As far as I can see the best way would be to just not extend the classes and do the functionality themselves.

Removing 8.3.0 release notes for now, since this is not in 8.3.0 yet.

Adding rerolled patch. Only conflict while rebase was deleted file ShortcutForm.php.

Thanks, for the reroll.

+++ b/core/modules/shortcut/src/Form/ShortcutAddForm.php
@@ -0,0 +1,36 @@
+    $message = $this->t('Added a shortcut for %title.', array('%title' => $view_link));
...
+    $form_state->setRedirect('entity.shortcut_set.customize_form', array('shortcut_set' => $entity->bundle()));

+++ b/core/modules/shortcut/src/Form/ShortcutEditForm.php
@@ -0,0 +1,36 @@
+    $message = $this->t('The shortcut %link has been updated.', array('%link' => $view_link));
...
+    $form_state->setRedirect('entity.shortcut_set.customize_form', array('shortcut_set' => $entity->bundle()));

We are not using short version of array now.

Thanks Jibran, corrected your mentions.

Thanks for the fixes. Please also upload the interdiff.

1. +++ b/core/modules/shortcut/src/Form/ShortcutAddForm.php
   @@ -0,0 +1,36 @@
   +      $view_link = \Drupal::l($entity->getTitle(), $url);
   
   +++ b/core/modules/shortcut/src/Form/ShortcutEditForm.php
   @@ -0,0 +1,36 @@
   +      $view_link = \Drupal::l($entity->getTitle(), $url);
   

   use of l is deprecated let's use $this->getLinkGenerator()->generate() instead.

2. +++ b/core/modules/shortcut/src/Form/ShortcutAddForm.php
   @@ -0,0 +1,36 @@
   +    $message = $this->t('Added a shortcut for @title.', ['@title' => $view_link]);
   +    drupal_set_message($message);
   
   +++ b/core/modules/shortcut/src/Form/ShortcutEditForm.php
   @@ -0,0 +1,36 @@
   +    $message = $this->t('The shortcut @link has been updated.', ['@link' => $view_link]);
   +    drupal_set_message($message);
   

   Let's remove $message variable.

@jibran, Adding suggested correction 1 & 2. However for interdiff if I get it correct I need to provide interdiff b/w #39 & #46.

But #39 seems to be corrupt, could you please guide? Really appreciate your guidance on it.

Thanks for updating the patch. @dev.patrick you can create an interdiff from #42 in #44 also you can create interdiff for #46 form #44.

I have created the change notice https://www.drupal.org/node/2920117.

@jibran, thanks for reviewing, uploaded both interdiff as asked.

@dev.patrick those are not correct these are correct.

Hmm, am doing lots of mistakes :( Appreciate your time for correcting and guiding.

Let's deprecate the old class rather than removing it, per https://www.drupal.org/core/deprecation#internal. (This is what #30, #33, #34 above are discussing.) Thanks!

For #51.