Inline deletion of shortcut should not be confirmed

Awesome! Yeah, this idea came up in passing in a couple other issues, but no one actually tried to write a patch. Thanks for starting this.

A couple comments:

• I don't think we should remove the confirm form in the case where the shortcut is deleted via the standard admin UI; the confirm form is actually quite expected there, and the delete action is much less trivial to undo when done from that screen. Therefore, I would leave the existing code rather than removing it, and have shortcut_link_delete_inline() invoked from a new menu callback that is only used for the "(-) Remove from shortcuts" button.
• The patch currently contains a CSRF vulnerability. To prevent that it needs a token in the URL - similar to the way shortcut_link_add_inline() does it.

Also, one more thing - not an actual problem with the patch, but more of a bonus thing that would be neat to have (could also be a followup issue)... I think that if you delete a shortcut by clicking the '-' icon and immediately "undo" it by adding it back with the '+' icon on the next page load, it should restore itself to the same place in the shortcut list that it was before (currently it will be added back at the end of the list instead). This could be done pretty simply by allowing shortcut_link_add_inline() to pull an optional 'weight' parameter from the URL, and potentially passing that along in the session or via other means for that one page load.

As suggested by David:

- added token to prevent CSRF vulnaribility
- uses new callback.

The restory/undo functionality could be for another issue imo.

Gonna RTBC it, I tested it and it worked.

Moving to 8.x, but this sounds like something we could probably backport. Tagging.

I'd love an extra set of eyes on that CSRF protection code and ensure that it's copasetic.

I reviewed the patch, and the CSRF protection code looks as copasetic as can be :) Basically, it's just standard use of drupal_valid_token() (and the same exact pattern used in the equivalent function for adding a shortcut).

However, it looks like there are two other problems:

1. The access callback seems like it should be 'shortcut_link_access' rather than 'shortcut_set_edit_access' - otherwise, the wrong kind of object is getting passed and it won't work correctly.
2. The text "Delete shortcut for %title" has the wrong grammar (should be "deleted", and also for %title doesn't seem right since the %title is the thing that was deleted). Actually, since we are talking about backporting this to D7, I'd suggest just using "The shortcut %title has been deleted." (That's an existing string already in the module, so we wouldn't be hurting translations.)

Addressed the two points in a new patch, setting to review again.

Patch from #9 worked fine for me, both in 8.x and 7.x installations
Test steps:

1. Fetch 8.x-dev branch - git clone --branch 8.x http://git.drupal.org/project/drupal.git d8
2. Fetch 7.x-dev branch - git clone --branch 7.x http://git.drupal.org/project/drupal.git d7
3. Create virtual servers in apache, d8.loc pointing to d8 and d7.loc pointing to d7
4. Install standard installation profile in d8.loc
5. Install standard installation profile in d7.loc
6. In admin menu, choose 'Configuration' > 'Development' > 'Performance'
7. Press + sign near the word 'Performance' to add performance screen to shortcuts
8. In shortcuts line, press 'Appearance'
9. Press - sign near the word 'Appearance'
10. Expected result: confirmation screen
11. Apply patch - patch -p1 < ~/tmp/791072-9.patch
12. In shortcuts line, press 'Performance'
13. Press 'Clean all caches'
14. In shortcuts line, press 'Appearance'
15. Press - sign near the word 'Appearance'
16. Expected result: 'Appearance' link is deleted from the shortcuts list
17. Steps 6-16 repeated for both d8.loc and d7.loc

Patch from #9 worked fine for me, both in 8.x and 7.x installations

+++ b/modules/shortcut/shortcut.admin.incundefined
@@ -768,3 +768,21 @@ function shortcut_link_add_inline($shortcut_set) {
\ No newline at end of file

Minor thing but there's a missing newline here.

Otherwise, it appears the changes David asked for in #8 have been made, and the patch has been tested functionally in #10.

There is no single newline in any file in core, so this one is good to go.

Here's a version without the newline thing (note the issue is missing newline characters, not actual newlines, I think - those can mess up future patches).

I reviewed also and agree it's RTBC.

+++ b/modules/shortcut/shortcut.admin.inc
@@ -778,3 +778,21 @@ function shortcut_link_add_inline($shortcut_set) {
+ * @param $menu_item
+ *   Returned from menu_item_load().
...
+function shortcut_link_delete_inline($menu_item) {

Variable name should be $link, and the loader is actually menu_link_load(), not item.

+++ b/modules/shortcut/shortcut.admin.inc
@@ -778,3 +778,21 @@ function shortcut_link_add_inline($shortcut_set) {
+  return drupal_access_denied();

Should return MENU_ACCESS_DENIED instead.

+++ b/modules/shortcut/shortcut.module
@@ -132,6 +132,15 @@ function shortcut_menu() {
+  $items['admin/config/user-interface/shortcut/%menu_link/delete-link-inline'] = array(

@@ -672,9 +681,9 @@ function shortcut_preprocess_page(&$variables) {
-      $link_path = 'admin/config/user-interface/shortcut/link/' . $mlid . '/delete';
+      $link_path = 'admin/config/user-interface/shortcut/' . $mlid . '/delete-link-inline';

Why is this a separate menu router item, and why does it break the hierarchical structure?

20 days to next Drupal core point release.

The csrf protection in the most recent patch (and at least several of the others) is appropriate. Removing the "needs security review" tag.

Udated the patch per sun's remarks.

Also changed the drupal_access_denied() to the same constant in shortcut_link_add_inline().

As for the separate menu router item and structure. Well it follows the add-link-inline menu item. When adding a link, we need to know the current shortcut set, when deleting we only need to know the mlid. Granted, this could all be in one single page callback, but that would be messy code in my opinion.

#17 looks good to me overall. Can we add tests for this?

Outside of that, a couple comment nitpicks:

+++ b/core/modules/shortcut/shortcut.admin.incundefined
@@ -775,5 +775,23 @@ function shortcut_link_add_inline($shortcut_set) {
+ * Menu page callback: deletes a link from a shortcut set.
+ *
+ * After completion, redirects the user back to where they came from.

The first line should be:
Page callback: Deletes a link from a shortcut set.
Also, I'd say "redirects the user to the original page" rather than "where they came from"--the latter is a bit odd.

#17: 791072-17.patch queued for re-testing.

Retesting against latest HEAD since it has been over a year.

@xjm: I am not sure what test can check here

This test probably doesn't work at all but it was all I could throw together for GCI 2013. Probably best to ignore it.

This would be a very nice UX improvement :)

I am going to try to look at this problem.

The confirmation screen is no longer shown when using the delete quicklink.
Instead of using a new test it was also possible to slightly change the test testShortcutQuickLink.
The delete confiirmation form remains unchanged when triggered by the operation link in the manage form.

+++ b/core/modules/shortcut/src/Controller/ShortcutController.php
@@ -34,4 +35,29 @@ public function addForm(ShortcutSetInterface $shortcut_set) {
+      entity_delete_multiple('shortcut', array($shortcut->id()));

Why not just do $shortcut->delete()?

Other than that, this looks good to go :)

Thanks for the review.

The function now uses $shortcut->delete()

Lovely, thank you! :)

This is now good to go.

P.S.: you even hid the previous patch — awesome, you're getting up to speed in best practices super fast! :D

@throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
The method does not actually throw any exceptions, and any exceptions thrown by the storage controller are caught.

Also, are the links and buttons to the delete page now marked with a danger? We should make sure it's clear clicking those results in a destructive action straight away.

@throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
The method does not actually throw any exceptions, and any exceptions thrown by the storage controller are caught.

Good catch!

@Danny.Wouters: could you remove that one line? :)

Also, are the links and buttons to the delete page now marked with a danger? We should make sure it's clear clicking those results in a destructive action straight away.

Please actually try the patch. This is for the inline "delete shortcut" link, i.e. when you click the star icon.

Thanks for the review.

I removed the line and created a new patch.

#30.1 is fixed, back to RTBC.

We need some tests - no?

+++ b/core/modules/shortcut/src/Controller/ShortcutController.php
@@ -34,4 +35,27 @@ public function addForm(ShortcutSetInterface $shortcut_set) {
+   *
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   *   A redirect response to the front page, or the previous location.
+   */
...
+    return $this->redirect('<front>');

The comment does not true?

The comment on the function is changed.

There are some tests in the function testShortcutQuicklink

  /**
   * Tests that the "add to shortcut" and "remove from shortcut" links work.
   */
  public function testShortcutQuickLink() {
    ...

    // Test the "Remove from shortcuts" link.
    $this->clickLink('Cron');
    $this->clickLink('Remove from Default shortcuts');
    $this->assertText('The shortcut Cron has been deleted.');
    $this->assertNoLink('Cron', 'Shortcut link removed from page');

    $this->drupalGet('admin/structure');
    $this->assertNoLink('Cron', 'Shortcut link removed from different page');
  }

Are these tests sufficient or do we need more tests?

+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -104,7 +104,6 @@ public function testShortcutQuickLink() {
-    $this->drupalPostForm(NULL, array(), 'Delete');

This line in the patch adapts the existing test and makes sure that a) the delete confirmation page is gone, and b) the shortcut link is still removed as it should be, so I think we have enough test coverage?

Leaving

+++ b/core/modules/shortcut/src/Controller/ShortcutController.php
@@ -34,4 +35,27 @@ public function addForm(ShortcutSetInterface $shortcut_set) {
+   *   A redirect response to the front page.

Does it always redirect to front, or only if not 'destination' query parameter is set?

@mr.baileys: Indeed, the function redirects to the page where the user clicked on the quicklink (to delete the shortcut to that page).

Is there a function to verify what the destination parameter is?

Some minor changes were made to re-roll the patch.
shortcut.module
$route_name = 'entity.shortcut.link_delete_inline';
shortcut.routing.yml
entity.shortcut.link_delete_inline

The comment in the function deleteShortcutLinkInline in ShortcutController.php is changed

   * @return \Symfony\Component\HttpFoundation\RedirectResponse
   *   A redirect to the previous location or the front page when destination
   *   is not set.
   */

After 4+ years let's get this committed.

Wow! :D I didn't realize that!

Thanks for seeing this through, Danny.Wouters!

Committed e023a68 and pushed to 8.0.x. Thanks!