Shortcuts should handle missing routes elegantly

Confirming that this happens with PHP 5.5.10 under MAMP.

Here's what I got from the log:

 Marker - May 31, 2014, 8:40:01 PM
[31-May-2014 20:40:18 America/Chicago] Uncaught PHP Exception Drupal\Component\Plugin\Exception\PluginNotFoundException: "The "node" entity type does not exist." at /Users/paulmitchum/projects/drupal8/core/lib/Drupal/Core/Entity/EntityManager.php line 199
[31-May-2014 20:40:18 America/Chicago] Exception thrown when handling an exception (RuntimeException: Failed to start the session because headers have already been sent by "/Users/paulmitchum/projects/drupal8/core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Response.php" at line 359.)
[31-May-2014 20:40:19 America/Chicago] Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "node.add_page" does not exist." at /Users/paulmitchum/projects/drupal8/core/lib/Drupal/Core/Routing/RouteProvider.php line 150
 

And here is a totally uninteresting screen shot:

Installing a new d8 standard profile, I run the cron and then uninstall the History module and I have this error.

Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "update.module_install" does not exist." at /Users/marthinal/workspace/drupal8/core/lib/Drupal/Core/Routing/RouteProvider.php line 150

I will try take a look at this tomorrow again.

@Xano It was impossible to avoid this error to me with this patch. Applying the patch + "drush pmu node" I had each time the error but for "All Content" Shortcut.

Making a grep I can verify that hook_modules_uninstalled() is always added to .module or and .inc. Changing the place for the hook the bug is fixed to me.

I was testing manually and also I made a quick test using the standard profile reproducing steps but I'm not sure why never fails ...

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -438,3 +439,23 @@ function shortcut_toolbar() {
   +function shortcut_modules_uninstalled(array $modules) {
   

   Somewhere in this function there should be a comment saying what this hunk of code is doing.

2. +++ b/core/modules/shortcut/shortcut.module
   @@ -438,3 +439,23 @@ function shortcut_toolbar() {
   \ No newline at end of file
   

   :-(

Not downgrading, though.

It's nice to solve the immediate problem, but if I were to reinstall node module, I'd want the 'add content' shortcut to come back. That might be outside the scope of this issue, so retaining RTBC.

#13 we need to document what is going on shortcut_modules_uninstalled()

Hi! It seems I have a similar problem but it appeared once I've deleted recently created view: "Uncaught PHP Exception Drupal\Component\Plugin\Exception\PluginNotFoundException: "The "views_block:bloki-block_1" plugin does not exist." at /.../core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php line 57". Any solution to this issue? Thanks!

17: drupal_2277753_17.patch queued for re-testing.

So rtbc again.

I'm pondering whether this is the correct solution. Deleting user created content (the shortcuts) silently seems like it might be a bad idea.

The other option is to catch the exception... and not print the broken shortcut.

We do a similar link removal in menu_ui when nodes are deleted, but it's broken right now, see #2248643: Notice thrown when checking access for menu link to non-existing node.

Removing the shortcuts seems fine, we uninstall all the other data related to nodes too. However, we should probably also prevent that an exception there can kill the site completey and should just not display those links? When we check access for menu links, we also catch several different exceptions and consider non-existing routes as inaccessible.

=> Crosspost with @alexpott, I think we should do both.

So let's ensure that if a route is missing that the exception is handled gracefully with this issue.

Beside of the other problem whether this is the correct solution in the first place.

+++ b/core/modules/shortcut/shortcut.module
@@ -453,3 +454,24 @@ function shortcut_toolbar() {
+  foreach (entity_load_multiple('shortcut') as $shortcut) {
+    /** @var \Drupal\shortcut\ShortcutInterface $shortcut */
+    try {
+      $route_provider->getRouteByName($shortcut->getRouteName());
+    }
+    catch (RouteNotFoundException $e) {
+      $shortcut->delete();
+    }

Did you considered using getRouteByNames instead?

@dawehner I don't think removing shortcut's on a missing route is right - we could just be in the middle of re-creating a view or something like that.

The patch attached would allow an administrator to fix the shortcut after removing a view or something like that.

+++ b/core/modules/shortcut/shortcut.module
@@ -453,3 +457,24 @@ function shortcut_toolbar() {
+ * Implements hook_modules_uninstalled().
+ */
+function shortcut_modules_uninstalled(array $modules) {
+  // Delete all shortcuts that point to routes of the uninstalled modules.

So why it is still part of the patch?

@dawehner because it is a good idea to clean up modules provided by routes on uninstall of a module - but we have to cope with dynamic routes differently.

@dawehner and #28

=> Crosspost with @alexpott, I think we should do both.

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -453,3 +457,24 @@ function shortcut_toolbar() {
   +  foreach (entity_load_multiple('shortcut') as $shortcut) {
   

   Can we use Shortcut::loadMultiple now?

2. +++ b/core/modules/shortcut/shortcut.module
   @@ -453,3 +457,24 @@ function shortcut_toolbar() {
   +    /** @var \Drupal\shortcut\ShortcutInterface $shortcut */
   +    try {
   +      $route_provider->getRouteByName($shortcut->getRouteName());
   +    }
   +    catch (RouteNotFoundException $e) {
   +      $shortcut->delete();
   +    }
   

   Did we considered to use getRouteByNames instead?

It looks like the patch in #40 only reflects a dependency bump-up.

This is just a symptom of #2346189: Denormalizing paths into route names/parameters is brittle / broken

Sorry double post.

To the issue:

- Lets please never ever delete user entered data automatically. That is one of the biggest UX no-go's.
- Disable them, put them in passive configuration, whatever, but not delete ...

If we need to remove automatically added short cuts, such config entities need to be exported to code, so they go away automatically if the config is unexported (if CMI supports that).

#44:

This is just a symptom of #2346189: Denormalizing paths into route names/parameters is brittle / broken

Yes and no.

There is two things to this issue:

a) route generation during link creation => FATAL
b) Trying to hit a removed route (from a bookmark or such).

Lets take a look at D7 first.

- I installed awesome_module and it created a path awesome/admin
- That was so awesome that I did create a short cut for it, so I now have shortcut_1 with name 'Awesome admin' pointing to awesome/admin

- But now I found out that awesome module had a critical security issue, so I immediately disable it.

Not awesome! :-p

- Now we have the exact same situation as in Drupal 8 (when using the scenario of uninstalling node).

I will now have a short cut called 'Awesome admin' that links to awesome/admin

But if I go there, I get a 404.

---

But is that good UX?

In my opinion: Nope

- It would be much better if the shortcut was still available on the admin, but marked as (invalid) and not showing up on the front end at all, so not only pointing the link to /sorry/this/link/no/longer/exists, but instead not showing the item at all.

What currently happens in Drupal 8 is that we get an Exception during the generation, which is fine as it is important for the caller to react to this exception.

When handling that Exception it can then remove the item from the front end or it can show that the link is invalid, so we gain valuable information from that.

We could do the same in Drupal-7 to check if a link is valid, but it is a little harder.

The BIG change is:

- Anytime a link or url is output you can get an Exception, while in D7 the invalid link was happily printed.

The next question then is:

Is it better to hide errors? And what possibilities does e.g. twig code have to know if an url it was passed or it is trying to create is valid?

Do we need new functions like:

routeIsValid() that is not throwing an exception and fallback to /sorry/this/route/does/not/exist as path by default?

This still allows interested parties to check the route before doing something, but would default to be safe to output by default.

Or should such a fallback e.g. only be implemented in the twig functions for link / url generation.

I think this is the bigger discussion to have.

Note that #2346189-29: Denormalizing paths into route names/parameters is brittle / broken solves this rather powerfully: whatever the user enters gets rendered as a link. Clicking the link might land on 403/404 but whatever the user enters is used (perhaps aliased but still). A followup is suggested where the formatter calls the access checker and then broken links won't render. This issue IMO can be closed as LinkItem will replace the custom Shortcut implementation for sure.

Now a duplicate of #2235457: Use link field for shortcut entity.