Rest plugins can cause fatal's during route rebuild

While #2589967: Rebuild routes immediately when modules are installed is about installation, is this related to the fact that the actual uninstall of the module vs. the route rebuild happen out of sync?

Given enabled resources come from config, how is the route rebuild happening before the config gets updated?

Its a general problem that we haven't thought at all about when we catch our exceptions and when not.

When uninstalling a module from drush I was left with a broken site with a exception being thrown during a route rebuild. Drush could probably help but the route rebuilder should be more resilient as well since there are many reasons a plugin could go away.

This sounds like we forgot the rebuild the router when we uninstall in Drush?

Just looking at the code, this could benefit from calling hasDefinition before trying to get the instance, also createInstance instead of getInstance.

Eclipse

I think I'd prefer to check for the existence of the plugin before instantiating, rather than catching the exception. This is a small part of route rebuild so an extra function call or three isn't going to really hurt anything. However, will this also self-clean on the next rebuild (ie, it only happens during the rebuild right after the module is disabled) or does it indicate we have stale config data lying around?

Also, #2579235: Resource plugin manager needlessly calls wrong method to instantiate plugins touches the same code, and if we're messing with getInstance() here, we might as well remove that call in favor of createInstance() and simplify the patch in 2579235 further.

Eclipse

I'd like to know why we hit this though - it could be missing config dependencies, it could be drush rebuilding the router before the config item gets updated.

Just ran into this myself. It does not appear to be a problem with drush as it is enough to simply visit rebuild.php after uninstalling your module.

It seems drupal_rebuild() calls drupal_flush_all_caches() which calls the rebuild method of the 'router.builder' service. This then calls the rest module's route subscriber which reads rest.settings from config and contains the plugin id for the now uninstalled plugin.

re: #7
Since rest.settings is not cleared during a rebuild, this means that we have stale data sitting around, which also means that all future rebuilds will end with a fatal error as well.

As for the patch, here are some quick-fix options (long-term: use config entities/dependencies for this instead?), sorted by preference:

1. The rest module should remove plugins from other modules when they are uninstalled so that rest.settings is always up to date.
2. Include a try-catch block in the route subscriber (like the patch in #2 does), but also remove the plugin from rest.settings since there is no way to fix the problem without the user reinstalling the source module. Until the router is rebuilt, rest.setting will contain invalid data however.
3. Have all contrib modules remove their own plugins from rest.settings. This is obviously a pain for all those modules...

Pending #8 and #13.

I think #13.1 ought to be enough without #13.2 - if it's not that's another problem.

Bumping to major - for me this is borderline critical - it's both a fatal error via the UI and a data-integrity issue, given modules providing plugins in general can expect to have them correctly handled by configuration dependencies for cases like fields/views etc you'd not expect to be manually removing things from other people's config objects.

I think this is a issue with rest.settings.yml being a simple config file - it should have been broken up into a config entity per entity type and then dependencies would have been the solution. But I think we're well past that point. The simplest solution to problem would seem to be that the Rest module should implement hook_modules_uninstalled to remove entity types when a module that supplies them is removed.

Yes #16 is exactly my thoughts here.

#15 didn't actually bump this to major, doing that now.

@neclimdul fair enough but we also need to clean up the cause of this particular problem - rest.settings.

Under the circumstances I'd support both trying to clean up the data (as described in #16) and catching the exception. But the exception, if caught, should log rather than just swallow silently as it indicates there is still a problem to be resolved.

Had a few minutes to spare. Includes changes from #13.1/#16 and an if-block version of #2 with the added log warning as per #20. Still needs tests.

Other notes:
Only module uninstalls will trigger the clean up of rest.settings, so it's still a pretty small band-aid over the data-integrity part of the issue.
The other log messages in alterRoutes() probably don't work because the placeholder and replacement keys are different?!?!

+++ b/core/modules/rest/src/Routing/ResourceRoutes.php
@@ -7,6 +7,7 @@
+use Drupal\Component\Plugin\Exception\PluginNotFoundException;

Should be removed.

+++ b/core/modules/rest/src/Routing/ResourceRoutes.php
@@ -68,7 +69,12 @@ protected function alterRoutes(RouteCollection $collection) {
+        $this->logger->warning('The %id resource no longer exists.', ['%id' => $id]);

If we're not going to let the exception get thrown (although I still don't see why we wouldn't during development), this should be a trigger_error() so that the error is visible and causes test failures.

Adding steps to reproduce to the IS.

1. +++ b/core/modules/rest/rest.module
   @@ -27,3 +27,22 @@ function rest_help($route_name, RouteMatchInterface $route_match) {
   +  foreach ($resources as $id => $methods) {
   +    if (!isset($definitions[$id])) {
   +      unset($resources[$id]);
   +    }
   +  }
   

   You could simplify this code by using $resources = array_intersect_key($resources, $definitions);

2. +++ b/core/modules/rest/src/Routing/ResourceRoutes.php
   @@ -7,6 +7,7 @@
   +use Drupal\Component\Plugin\Exception\PluginNotFoundException;
   

   we no longer need this

Huh, was going through my issue feed and it seems like I came across this problem 5 months ago and forgot about it. This issue is actually a duplicate of #2397271: REST configuration fails if it contains a plugin reference that does not exist (but the patch there only fixes the fatal error).

That issue also lists another interesting way to reproduce the problem though. Apparently you can't install a profile that contains a rest.settings.yml file with settings for plugins defined by a custom module.

As per #9 in that issue, it also means that we shouldn't clean up rest.settings in the route subscriber, and should add a comment to that effect in whichever patch is chosen.

I also prefer the try/catch - it's a bit more honest about what's happening, given this is an invalid state.

This is indeed a duplicate of #2397271: REST configuration fails if it contains a plugin reference that does not exist.

And the eventual solution we want is #2308745: Remove rest.settings.yml, use rest_resource config entities.