Improperly deployed *missing* entity table breaks module uninstall

Man that's annoying. Not being able to uninstall a broken module is a pretty big deal IMHO.

Looks fine, now we need a test. We should have plenty of uninstall tests, lets duplicate something and then we manually remove the table first.

Why I consider this to be major:

This typically happens when you create an entity with a new entity type. You add it, but drupal doesn't know about it. So you try to re-install your module. But that fails because it now tries to remove the entity tables that were never created.

There are pretty easy workarounds, like temporarily removing your entity class, or writing an update function or just running the necessary drush commands. But I've seen new developers getting stuck on this problem.

I think I got stuck on this for a while actually while developing a custom entity where I made a typo and broke everything. Took me a while to get the magic set of commands and changes to recover. Pretty miserable DX.

This fix seems reasonable to me; if a module does not have a schema installed then it does not have data, so it's logical to catch that specific exception only and return false.

There is a slight chance of disruption here if extending code were already responding to SchemaObjectDoesNotExistException, so if we are strict we might want to move this to 8.3.x. However, it's much less disruptive than (e.g.) throwing a new exception or catching exceptions more broadly, so we could discuss whether to backport it to a patch release if needed, but I don't think it's needed since this is a DX improvement that helps protect data integrity in case of miswritten code.

Still needs tests though. They should be relatively straightforward and can be based on an existing test to uninstall a module. It's a slightly advanced novice task: as described in #6, find an existing test that tests uninstalling a module, copy it, add a couple lines that manually drop the database table, and test that the module is still uninstalled successfully. We should also potentially add unit tests for catching the exception.

Discussed with @xjm, @alexpott, @amateescu, @fago, @berdir and @plach. We
agreed that this is major because it is painful DX. Plus, it is easy to fix.

Not being able to uninstall a module in this scenario can also be problematic for data integrity (in addition to it being a real blocker for developers).

Test is not a in perfect place but works.

Noticed that the exception used there is only used in schema operations, it's just a standard database exception here.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -66,10 +67,18 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
+      // An exception here means that the table does not exist. That means
+      // there is no data. This exception is catched to allow to uninstall
+      // modules with incorrectly installed entity types.

s/catched/caught

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -66,10 +67,18 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
+    try {
+      return (bool) $this->getQuery()
+        ->accessCheck(FALSE)
+        ->range(0, 1)
+        ->execute();
+    }
+    catch (DatabaseExceptionWrapper $e) {
+      // An exception here means that the table does not exist. That means
+      // there is no data. This exception is catched to allow to uninstall
+      // modules with incorrectly installed entity types.
+      return FALSE;
+    }
   }

For the sake of discussion, is there anything else that can happen that would throw a DatabaseExceptionWrapper here?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php
@@ -66,10 +67,18 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
+    catch (DatabaseExceptionWrapper $e) {

We are in the storage base class here, so we should not assume a database.

To alleviate @mpdonadio's concern, I'm wondering whether we could leave the entity query in place here (with no try/catch) and override it with a sql query in the SQL storage to minimize the possibility a different exception is unintentionally caught.

"Slightly advanced novice task", see comment 12.

Doesn't apply to 8.4.

Re-rolled with slight fuzz.

I don't know how the sql query override in the 2nd part of #18 would be implemented. If we are going with this patch, need to make the change suggested in #17. And, I think that the first part of #18 is suggesting to use a different Exception type?

Related: Why wont drupal recognize my base table name?

Patch in #23 no longer applies - re-rolled.

Corrected spelling mistake identified in #17.

Tests already added in #15.

There is no longer a novice task.

This is a nice, simple solution that is well documented. It has a test that is also nicely documented. I queued up tests against all three database backend, and they came up green. #15 showed a test-only patch to demonstrate the problem and solution. This should be good to go.

Queueing up 8.4.x tests, too, since this should be eligible for that.

Just re-read this whole thread. The comments in #18 weren't discussed or addressed. So back to NW :(

I just encountered this error on our install. Running 8.8.5. I re-rolled patch for this version.

Adressing #18.

Thank you for addressing the feedback @johnwebdev patch looks great.

@plach also suggested We are in the storage base class here, so we should not assume a database. in #18. Now that we are in SqlContentEntityStorage where DatabaseExceptionWrapper is already used we don't need to change the Exception.

Given #18 is the only outstanding feedback and we have red and green patches I think this is RTBC.

I think this is fixing this in the wrong location. I don't think that by default hasData() should be robust to sql error - if it experiences an error when called we should continue to throw an error - however I think there is a clear case for doing that in uninstall. So I think we should be catching the database exception in \Drupal\Core\Entity\ContentUninstallValidator::validate() and allowing uninstall to proceed in those cases.

I can confirm this patch works on Drupal 8.8.2. I can see the module uninstall page again.

I will work on it

Hello everyone,
I did a new patch with what @alexpott pointed on comment #43.

It is good to know that if the module has more than one entity, it will be able to uninstall the module even if one table from one entity is dropped and the other is not.

Cheers, Paulo.

The correct interdiff.

1. +++ b/core/lib/Drupal/Core/Entity/ContentUninstallValidator.php
   @@ -40,12 +40,19 @@ public function validate($module) {
   +      } catch (\Throwable $th) {
   

   Was going to ask for something more specific to catch, but since this is moved to ContentUninstallValidator we are back to @plach' point in #18 that we can't assume a database

2. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntitySchemaTest.php
   @@ -349,6 +349,10 @@ public function testCleanUpStorageDefinition() {
   +    // Manually drop one of the test entity type tables to simulate a scenario
   +    // where a table was not created or an entity type is new.
   +    \Drupal::database()->schema()->dropTable('entity_test');
   

   As @Berdir pointed out when he added this, this might not be in the best place? With this addition we are now only testing the uninstall when there are missing tables and no longer testing it when all tables are present (which seems to me like the more common scenario). Maybe add a test case where we are removing the table but leave the existing test case as it is?

Patch #46 works for me on 9.1.5.

Setting to NW for #48

As per second review comment in #48, Moving the test to a new function. I don't understand what the first suggestion in #48 or #18 means.

I found the following code in Drupal\mysql\Driver\Database\mysql\ExceptionHandler

      // SQLSTATE 23xxx errors indicate an integrity constraint violation. Also,
      // in case of attempted INSERT of a record with an undefined column and no
      // default value indicated in schema, MySql returns a 1364 error code.
      if (
        substr($exception->getCode(), -6, -3) == '23' ||
        ($exception->errorInfo[1] ?? NULL) === 1364
      ) {
        throw new IntegrityConstraintViolationException($message, $code, $exception);
      }

      throw new DatabaseExceptionWrapper($message, 0, $exception);

SQLSTATE error code is checked here and if its in 23xxx pattern, IntegrityConstraintViolationException is thrown.

Similerly, "Base table or view not found" Error is having the error code "42S02". So, Couldn't we check if the code is in '42xxx' pattern and throw a new exception 'TableNotFoundException' , and then catch it during uninstall? (But the doumentation https://docstore.mik.ua/orelly/java-ent/jenut/ch08_06.htm says that return code 42 is for Syntax error or access rule violation :[ )

Fixed CCF for #56.

#57 worked for me! Thanks!

Best,
Mariska

I installed Drupal 10.2.4 and started building the site, just regular site building stuff - enabling contrib modules and creating pages. Then I decided to uninstall the 'contact' module and clicking /admin/modules/uninstall threw this error,

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'custom_theme.media' doesn't exist: SELECT "base_table"."vid" AS "vid", "base_table"."mid" AS "mid" FROM "media" "base_table" GROUP BY "base_table"."vid", "base_table"."mid" LIMIT 1 OFFSET 0; Array ( ) in Drupal\Core\Entity\Query\Sql\Query->result() (line 271 of C:\wamp64\www\custom-theme\public_html\core\lib\Drupal\Core\Entity\Query\Sql\Query.php).

The 'media' module was auto-installed as a dependency for 'bootstrap_layout_builder' module. I have not added a media item yet.

Anyways patch #57 got me back to the Uninstall page and I was able to uninstall the 'contact' module.

Thanks @_utsavsharma

My issue wasn't solved. What happened in my case was somehow 'media' module did not install completely or something cos I found the media types were not available on the site and visiting 'admin/content/media' threw an error. So I rolled back the site till I uninstalled 'media' module. I removed patch #57 and re-installed the media module and it was now properly installed. I re-installed back all the other modules and site came back normal.

The thing is, without patch #57 I wouldn't have been able to access the Uninstall page to uninstall the media module and the other modules that required it.

I've converted this to an MR and reviewed it. It looks like the second question in #48 was taken care of in the latest patch.

I fixed some phpcs issues and updated the issue summary remaining tasks.
If someone can share some direction on #18 I'm happy to make an attempt.

I wonder if rather than just overriding the request and trying to narrow the scope, maybe in the catch we need to examine the exception and return false if it's due to a missing table, and throw the reason otherwise?

I don't see code in the current changes that assume a database. So, perhaps the comment in #18 is about surrounding code outside of these changes. I am going to mark this needs review again until it's more clear what work needs to be done on this MR.

So believe #18 was referring to the original fix at that time around ContentEntityStorageBase so would say may not be relevant with new approach?

Will leave in review but believe proposed solution may need to be tweaked.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I wonder if rather than just overriding the request and trying to narrow the scope, maybe in the catch we need to examine the exception and return false if it's due to a missing table

I don't think there's a specific-enough exception to look for which only covers missing tables, but we could narrow it down to DatabaseException?

The suggestion in #18 is that Database-specific exception handling shouldn't be done outside the SQL content entity storage class, and per #43, the exception handling was moved to the content uninstall validator.

At the very least, I don't think all the exceptions should be swallowed without at least logging an error or warning and perhaps also showing a message. Theoretically at least Url::fromRoute() and EntityTypeManager::getStorage() could throw exceptions as well.

An alternative could be something implementing hasData() in SqlContentEntityStorage to be something like this:

  /**
   * {@inheritdoc}
   */
  public function hasData() {
    try {
      return parent::hasData();
    }
    catch (\Throwable t) {
      throw new EntityStorageException($t->getMessage(), $t->getCode(), $t);
    }
  }

Then catch EntityStorageException in the uninstall validator.

EntityStorageException could be worth a look yeah.

At the very least, I don't think all the exceptions should be swallowed without at least logging an error or warning and perhaps also showing a message.

If we know this situation can only happen when an install goes wrong in the first place, an E_USER_WARNING might be good?

Amending attribution, and crediting the major triage:

Discussed with @xjm, @alexpott, @amateescu, @fago, @berdir and @plach. We