Problem/Motivation
It is possible to delete the default shortcut set by running
shortcut_default_set()->delete();
This leaves Shortcut module (and by extension the entire website) in a completely broken set. For example, just by visiting /admin/config/user-interface/shortcut or /user/%/edit you will see a fatal.
Proposed resolution
Thrown an exception when trying to delete the default shortcut set.
Specifically implement \Drupal\shortcut\Entity\Shortcut::preDelete(), loop over the passed $entities and check whether any of them has $entity->id === 'default' and if so, throw a \BadMethodCallException. This will also need tests.
Remaining tasks
User interface changes
None.
API changes
None.
Technically an added exception is an API change, but if someone tried to do this previously this was undoubtedly by accident and they would certainly have liked to be warned.
Original report by @chx
Although I made an oath to never use the default profile, testing uses it and spits notices at me. (Of course. That's why I do not use it.) I still keep my word tho and never actually look at it :p
| Comment | File | Size | Author |
|---|---|---|---|
| shortcut_set_empty.patch | 4.07 KB | chx |
Comments
Comment #1
David_Rothstein commentedHow did you wind up with no shortcut set? This shouldn't be possible without hacking the database; shortcut_current_displayed_set() is AFAIK always guaranteed to return something. I also haven't seen any notices running tests...
Can you explain more how to reproduce this bug?
Comment #2
yoroy commentedIf you delete all shortcuts from the default set, then save changes you get
Shown only on saving changes to the set, not on other admin pages.
Comment #3
catchLooks like a valid bug but this isn't critical.
Comment #4
David_Rothstein commented@yoroy's bug is definitely reproducible, but it's not the same as the original issue. The issue @yoroy reported here was later reported at #937380: errors when submitting the shortcut set configuration page after deleting all shortcuts, and since that is a dedicated issue for it, I posted a patch to fix it there.
I'm setting this one to "needs more info". I still don't know how to reproduce the original bug report except by hacking the database.
Comment #5
yoroy commentedBump to d8. I didn't verify if the issue still exists.
Comment #6
tstoecklerI found that it is in fact possible to delete the default shortcut set from the API, so repurposing this issue for preventing that.
Comment #7
xjmComment #18
catchWe don't really stop people from deleting things in the API usually, so moving this to a task. This could break code that's deleting the default, then adding a new default. Feels like it's really up to calling code to check first.
Comment #23
quietone commentedThe Shortcut Module was approved for removal in #3476880: [Policy] Move Shortcut module to contrib.
This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.
The deprecation work is in #3569117: [meta] Tasks to deprecate the Shortcut module and the removal work in #3569121: [meta] Tasks to remove the Shortcut module.
Shortcut will be moved to a contributed project before Drupal 12.0.0 is released.