\Drupal\Core\Entity\Sql\DefaultTableMapping::getAllColumns should check if the field storage definition exists

Test fails on Drupal 10.1.x. Restoring status.

This needs to be updated, remember to make a test only patch. Thanks.

Re-rolled 3064323-10-TEST-ONLY.patch against 10.1.x. Please review

The fix is now missing

Pach for 10.1.x

Thank you @bhaveshithape for the interest in this issue.

The last patch doesn't pass commit checks, could you make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel... →

Removing credit for #22 has it is expected the patch be checked before uploaded.

Leaving the needs reroll tag.

added updated patch and fixed CCF #22

1. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -176,7 +176,9 @@ public static function create(ContentEntityTypeInterface $entity_type, array $st
   -    $key_fields = array_values(array_filter([$id_key, $revision_key, $bundle_key, $uuid_key, $langcode_key]));
   +    $key_fields = array_values(array_filter([$id_key, $revision_key,
   +      $bundle_key, $uuid_key, $langcode_key,
   +    ]));
   

   We usually put this on same line, or each element in its line. I guess this was your IDE doing more than expected.

2. +++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
   @@ -329,7 +333,7 @@ public function getAllColumns($table_name) {
   +      if (isset($field_name, $this->fieldStorageDefinitions[$field_name]) && $this->requiresDedicatedTableStorage($this->fieldStorageDefinitions[$field_name])) {
   

   TIL isset accepts multiple arguments, nice change.

Removing needs reroll tag, as it applies. Added Novice, as the change requested is ok for someone to get started on contribution.

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -227,7 +227,9 @@ public static function create(ContentEntityTypeInterface $entity_type, array $st
-      $revision_base_fields = array_merge([$id_key, $revision_key, $langcode_key], $revision_metadata_fields);
+      $revision_base_fields = array_merge([$id_key, $revision_key,
+        $langcode_key,
+      ], $revision_metadata_fields);

Still same issue

This looks good to me.

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -227,7 +227,7 @@ public static function create(ContentEntityTypeInterface $entity_type, array $st
-      $revision_base_fields = array_merge([$id_key, $revision_key, $langcode_key], $revision_metadata_fields);
+      $revision_base_fields = array_merge([$id_key, $revision_key, $langcode_key], $revision_metadata_fields);

Dreditor is showing me a change here that I'm not able to identify, but applying the patch locally I don't see this as a changed line.

I'm reattaching the patch after applying it. Somehow is smaller, but don't see why. I guess that disallows me to RTBC.

Ran the tests locally without the fix and it failed as expected.

Change looks fine and I'll mark it. But we may want to figure out the why the storage is being deleted before the field. Will see the committers want to add this check though.

Yes, I'd be keen to get an answer to #16 or some steps to reproduce - tagging for issue summary update for that.

My concern is that this might be silently masking a larger problem

Yeah, sadly never had reliable steps to reproduce it. And 4 years later I don't even remember the context where this was happening.

I'm attaching #32 again to see if the only-test patch still fails.

As I'm the original poster and there is not a crazy activity here of people facing this, feel free to close (cannot reproduce) if you are not comfortable committing it without reliable STR.

The last submitted patch, 35: 3064323-35.only-test.patch, failed testing. View results →

Been keeping an eye on this one. Marking it to see what a committer thinks. But the test failure shows it could happen I suppose. Seems like a good idea to have the security blanket. Just my vote.

larowlan → credited lauriii → .

Discussed this with @lauriii

We both settled on marking this as closed won't fix, for the following reasons:

* We're adding a lot of test code that we have to maintain (test code isn't free) for what is essentially an edge case that we can't reproduce. The test itself is artificial and doesn't represent a real-world scenario
* The change being added, whilst making the code more defensive, is silently masking what would be a series data integrity issue - a field definition without a matching field storage definition is definitely something we don't want to quietly ignore.

So on that basis, marking this as won't fix.

If someone has steps to reproduce this or feels strongly, feel free to re-open.

Thanks everyone for their efforts here.