Issue originally opened at #2297817: Do not attempt field storage write when field content did not change and kept hostage for a year because of Drupal 8. Now that the Drupal 8 patch does not seems as obvious as it was and discussion is going further, I open a Drupal-7 only patch as per the backport policy states (actually it should be the other way around but the original issue is now too deep in Drupal 8 to change back).

This patch introduces a check in field SQL storage when updating or saving entities which prevents it from doing useless DELETE/INSERT when saving unchanged content. See details in the other issue, but this is a huge performance win when dealing with a high-frequency writes sites which has many fields (many start to less than 10 to see a difference).

CommentFileSizeAuthor
#86 2470619-86.patch8.31 KBmcdruid
#86 interdiff-2470619-81-86.txt1.7 KBmcdruid
#81 2470619-81.patch8.35 KBmcdruid
#81 interdiff-2470619-80-81.txt1.96 KBmcdruid
#80 2470619-80.patch7.46 KBmcdruid
#80 interdiff-2470619-66-80.txt3.12 KBmcdruid
#67 interdiff_49-66.txt1.29 KBheddn
#67 2470619-66.patch7.44 KBheddn
#49 interdiff.txt5.26 KBhosef
#49 drupal-stop-saving-unmodified-field-data-2470619-49.patch7.48 KBhosef
#36 drupal-n2470619-36.patch4.14 KBDamienMcKenna
#32 2470619-32-field_storage_ignore_unchanged.patch4.14 KBpounard
#24 field_storage_ignore_unchanged-2470619-24.patch3.84 KBboyan.borisov
#19 2470619-field_storage_ignore_unchanged.patch-custom.patch3.88 KBpounard
#13 field_sql_storage_module_-_html.png180.13 KBjoelpittet
#13 field_sql_storage_module_-_html.png151.35 KBjoelpittet
#1 2470619-1-field_storage_ignore_unchanged.patch3.67 KBpounard
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pounard’s picture

Status: Active » Needs review
FileSize
3.67 KB
Fabianx’s picture

Category: Feature request » Task
Status: Needs review » Reviewed & tested by the community
Fabianx’s picture

Priority: Normal » Major

And this is at least major for performance reasons.

Fabianx’s picture

Title: Do not attempt field storage write when field content did not change [Drupal 7] » [Drupal 7] Do not attempt field storage write when field content did not change

.

marcingy’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

This can't go in until fixed in d8.

marcingy’s picture

Status: Patch (to be ported) » Postponed
amateescu’s picture

Status: Postponed » Reviewed & tested by the community

It's fixed in D8 now :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: 2470619-1-field_storage_ignore_unchanged.patch, failed testing.

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Ok so this broke a commerce site:(

Try this patch and try to save a commerce_price field with a 0 int value for the amount.

Throws this as it seems to mutate the the 0 value to NULL:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'field_taxable_price_amount' cannot be null: INSERT INTO {field_data_field_taxable_price} (entity_type, entity_id, revision_id, bundle, delta, language, field_taxable_price_amount, field_taxable_price_currency_code, field_taxable_price_data) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8); Array
(
    [:db_insert_placeholder_0] => commerce_product
    [:db_insert_placeholder_1] => 10170
    [:db_insert_placeholder_2] => 10179
    [:db_insert_placeholder_3] => promotional
    [:db_insert_placeholder_4] => 0
    [:db_insert_placeholder_5] => und
    [:db_insert_placeholder_6] =>
    [:db_insert_placeholder_7] => USD
    [:db_insert_placeholder_8] => A:2:{s:11:"include_tax";b:1;s:10:"components";a:0:{}}
)

Also a nitpik:

+++ b/modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -372,6 +372,68 @@ function field_sql_storage_field_storage_load($entity_type, $entities, $age, $fi
+    return false;
...
+    return true;

Capitalize the boolean vars for coding standards.

joelpittet’s picture

+++ b/modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -372,6 +372,68 @@ function field_sql_storage_field_storage_load($entity_type, $entities, $age, $fi
+        $array[$language][$delta] = array_filter(array_intersect_key($item, $field['columns']));

I think this is somehow mutating the value but I don't understand why and can't create a MVC to reproduce:(

joelpittet’s picture

All I know is that is where the value gets dropped, (to be clear it's dropped after exiting _field_sql_storage_write_compare() on the $field_name = $field['field_name']; line after the comparison.

The amount is removed.

joelpittet’s picture

So you don't think I'm crazy(this may not stop you from thinking that):

Here's the script I used:
https://gist.github.com/joelpittet/233b866eddbd1c806784

I have a taxable_price field that and was updating an existing field, I'm sure that could be modified into a test somehow.

pounard’s picture

This is because of the array_filter() call, a callback must be provided as second argument that only exclude === '' and === null values, then your site will work again! Will fix that, thanks for finding this bug.

joelpittet’s picture

@pounard you likely have a better grasp of what's going on but why is the $entity->field's values changing as they are passed by value and not by reference to the comparison function, no?

That's what's freaking me out a bit and I can't come up with a MVC code in 3v4l to reproduce what I'm seeing as I step through the code.

Actually haha, you can see I just realized in my screenshot above that I tried cloning the entity before passing it in to subvert this bug. (Same thing happened with or without that hack)

Fabianx’s picture

#14: Is there anything that changed in D8 that we need to port back? I remember that the filtering was also moved somewhere else IIRC in D8 ...

pounard’s picture

#16 I'm not sure, it's just a matter of '0' values being filtered out, maybe can you have a look to what's been commit to D8 and look for potential array_filter() calls ?

Fabianx’s picture

#2297817-71: Do not attempt field storage write when field content did not change had the same observation on the filtering in equals, it was fixed in #72.

I think we may need to do another straight port of the D8 committed patch to get the remaining fixes after the fork ...

pounard’s picture

Straight backport seems a very difficult task, since the API is very different. I did some minor modifications to my original patch due to problems with Commerce, here is a SVN patch version (sorry can't use git from here). Posting here so that I can find it back when I'll have a real internet connection :)

Jordan Samouh’s picture

Hello Pounard,

We have this kind of problem
Thanks for this patch.

Did you see some others problems by appliyng this patch?
I see the status Needs work

Thanks.

pounard’s picture

We actually use this patch on 2 production sites for now quite some time, one commerce and another one more standard, still did not experience any bugs. The commerce one was prone to the "0" value error, but now seem to be working fine with the latest patch.

discipolo’s picture

just wanted to verify whether the expected result of this patch are less sql queries when saving nodes without changing fields? (as long as the node is not a new revision)

pounard’s picture

That's pretty much it, indeed.

boyan.borisov’s picture

Patch #19 works great for me but in some corner cases if I have broken/incomplete entity I got an error like this:

array_intersect_key(): Argument #1 is not an array [error]
File modules/field/modules/field_sql_storage/field_sql_storage.module, line 461
, in class="placeholder">sites/all/modules/contrib/migrate/includes/base.inc:707

I've created a patch where I do an additional check just before to use array_intersect_key()

mikeytown2’s picture

Status: Needs work » Needs review

Let's see what the testbot does

Status: Needs review » Needs work

The last submitted patch, 24: field_storage_ignore_unchanged-2470619-24.patch, failed testing.

The last submitted patch, 19: 2470619-field_storage_ignore_unchanged.patch-custom.patch, failed testing.

The last submitted patch, 24: field_storage_ignore_unchanged-2470619-24.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 24: field_storage_ignore_unchanged-2470619-24.patch, failed testing.

pounard’s picture

#19 patch does not apply because it's not a GIT patch, but an SVN-like patch, it needs to be applied manually using the patch -p0 < PATCHFILE command, then re-done using git diff > NEWPATCHFILE then the testbot will be able to apply it.

pounard’s picture

Status: Needs work » Needs review
FileSize
4.14 KB

Proper patch that should work. I added #24 minor modification.

pounard’s picture

Yay ! Reviews or RTBC please !

DamienMcKenna’s picture

Does this need additional tests, or is the fact that all tests pass sufficient to say it's ok?

pounard’s picture

Hum good question, I have to admit I would be more comfortable myself with a few additional tests, but if core did it right, it supposed to be okay. Nonetheless this patch still did encountered a few bugs without tests failing, such as not updating 0 values which were considered as empty by array_filter() hence the _field_sql_storage_write_compare_filter_callback() function, this means that core tests are not complete.

DamienMcKenna’s picture

DamienMcKenna’s picture

Status: Needs review » Needs work

Putting this back to "needs work" as it needs to be more precise in its comparisons, and needs tests to check edge cases.

pounard’s picture

Considering

as it needs to be more precise in its comparisons

I think it's ok as it is, but I do agree with tests.

ndobromirov’s picture

Issue tags: +scalability

Hi,

Is there any progress on this? This is quite the huge performance optimization, as sparing the write to a field table will spare also the query cache entries for it, this greatly improving MySQL's performance. Note that query cache is enabled by default in MySQL and depending on the traffic patterns it might cause more harm than help if there are more writes than not. This is still environment / project dependent, but still relevant note that affects scalability.

BR, Nick.

pounard’s picture

Real status is no one seems to care since Drupal 8 branch was created, but I'm patient :) I don't loose hope about this patch going in.

das-peter’s picture

We're using this patch for quite a while and I just came across the first issue.
If a "broken" entity is handled you might get a notice like: array_intersect_key(): Argument #1 is not an array.
File /var/www/html/modules/field/modules/field_sql_storage/field_sql_storage.module, line 461.
_field_sql_storage_write_compare_filter(), fairly, assumes that $item is an array - which not necessarily is the case if the field / entity / something is broken.
In my current case $item is NULL - now the question is how to resolve such a situation.
I suggest we simply skip the cleanup if $item isn't an array.

dreamer777’s picture

Does this solution work with field collection fields and items?

hosef’s picture

What kind of things need to have tests written for this to be ready? I can write the tests, but I don't know what tests need to be written.

hosef’s picture

It appears that the problem reported in #41 was fixed already in #24

Fabianx’s picture

#44:

The most important thing is that D8 changed some things about the approach and those need to be backported to D7 first.

Then we should backport the additional tests that D8 provided as well.

And then it can go in - possibly as opt-in experimental feature first.

hosef’s picture

Title: [Drupal 7] Do not attempt field storage write when field content did not change » Do not attempt field storage write when field content did not change
Version: 7.x-dev » 8.3.x-dev
Assigned: Unassigned » hosef

I ran a quick test, and found that Drupal 8 also runs a delete/insert for every field/property even if only 1 thing was changed, so this needs to be fixed in Drupal 8 as well.

I, also, found that the field module in Drupal 8 does not handle the actual saving of the data at all. That is handled in Drupal\Core\Entity\Sql\SqlContentEntityStorage, and the way it works is so different from Drupal 7 that backporting the Drupal 8 change does not look like a good idea.

For the Drupal 7 patch: The test coverage in the field_sql_storage module looks pretty complete already, so I would just need to write some tests for the comparison functions to make sure it compares complex fields correctly.

EDIT: I took a look at the code, and it does appear to check the field data before saving. The large number of queries I saw was because "create new revision" defaulted to true and I forgot to turn it off.

pounard’s picture

Agree, I don't think that D8 backport would be doable in any way.

hosef’s picture

Version: 8.3.x-dev » 7.x-dev

changing back to Drupal 7 because I was mistaken when examining if it was an issue in Drupal 8. :)

hosef’s picture

Here is a patch that fixes a few more coding standards things and also adds a new test in field_sql_storage.test.

Most of the scenarios that I check for in the test should probably not happen unless a module is really mangling the data, but we should handle them gracefully anyway.

Status: Needs review » Needs work

The last submitted patch, 49: drupal-stop-saving-unmodified-field-data-2470619-49.patch, failed testing.

hosef’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 49: drupal-stop-saving-unmodified-field-data-2470619-49.patch, failed testing.

hosef’s picture

Status: Needs work » Needs review

I'm not sure what is going on, but there are lots of test failures that are unrelated to fields and when I re-queue it different tests fail.

hosef’s picture

Issue tags: -Needs tests

Well, I re-queued it again and they all passed, so something weird must have been going on with one of the testbots.

hosef’s picture

Assigned: hosef » Unassigned
btully’s picture

Is this the latest working patch for D7? It does not apply cleanly against Drupal 7.54. Any advice on how to apply it?

oadaeh’s picture

@btully, I just applied the patch cleanly to a current dev branch of D7 checked out with Git, so it still does apply cleanly for the situation in which it was created.

If you are applying the patch with the patch utility and/or to a stable release packaged file downloaded from this site, maybe try executing this command:
patch -p1 --merge < drupal-stop-saving-unmodified-field-data-2470619-49.patch
I found it here: https://stackoverflow.com/a/30860581

oadaeh’s picture

Status: Needs review » Reviewed & tested by the community

I think the variables being checked should be before the value they are checked against.

+  return NULL !== $value && '' !== $value;
...
+    if (NULL === $original_vid) {

I think this setting could just be set before the conditional check. Then the variable will be set regardless of what happens and the else block will be unnecessary.

+  else {
+    $is_new_revision = FALSE;
+  }

However, those are just nit picks that do not affect the operation, and the other changes look fine to me.
The code changes have been in production for us since mid-December without any problems.

David_Rothstein’s picture

Issue tags: +Drupal 7.60 target
MustangGB’s picture

This is so nice, reduced my loading times on a badly performing page (ajax updating multiple hundreds of nodes) by something like 80%.

oadaeh’s picture

The patch still applies cleanly to the code changes for 7.55 & 7.56, and I just re-queued the tests to verify they all still work.

mxr576’s picture

Okay, so how we can give a boost to this patch to be committed to the core? What is remaining?

joseph.olstad’s picture

For those tired of waiting, I am going to make an install profile for D7 including this gem and all the other rtbc patches for 7.70 and possibly some extra performance patches, check my sandboxes in early to mid February. **UPDATE** the media_dev distribution has this patch on the latest 7.x-dev core branch and many others. ** END UPDATE **

hosef’s picture

I just tested it on my local machine against the latest D7 code base. It still applies, and all the tests still pass.

joseph.olstad’s picture

Issue tags: -Drupal 7.60 target +Drupal 7.70 target

Bumping to 7.70. This didn't make it into 7.60

Fabianx’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/modules/field/modules/field_sql_storage/field_sql_storage.module
@@ -435,6 +435,80 @@ function field_sql_storage_field_storage_load($entity_type, $entities, $age, $fi
+  return $items1 !== $items2;

This should be: != and not !== according to https://www.drupal.org/project/drupal/issues/2475237#comment-9879243

heddn’s picture

quietone’s picture

Only two items on wording.
Unfortunately, not confident with Drupal 7 to set this RTBC (and haven't read the full history because of that) :-(

  1. +++ b/modules/field/modules/field_sql_storage/field_sql_storage.module
    @@ -434,6 +434,80 @@ function field_sql_storage_field_storage_load($entity_type, $entities, $age, $fi
    +        // sometime giving invalid data to the field API.
    

    s/sometime/sometimes/, I think.

  2. +++ b/modules/field/modules/field_sql_storage/field_sql_storage.module
    @@ -434,6 +434,80 @@ function field_sql_storage_field_storage_load($entity_type, $entities, $age, $fi
    +  // field columns may stale in the structure items. We need to clean
    

    'may stale'?

joshmiller’s picture

Status: Needs review » Reviewed & tested by the community

We recently deployed a Drupal 6 to Drupal 7 migration where we used this patch and #2994212: field_sql_storage_field_storage_load does use an unnecessary sort in the DB leading to a filesort to speed up a 72+ hour migration to under an hour on the same hardware. For certain data intensive work, this is a must-have and works quite well at speeding up Drupal Commerce on the day-to-day work of a site as well.

MustangGB’s picture

Issue tags: -Drupal 7.70 target +Drupal 7.69 target

Seems like the whole major/minor release idea went out the window a long time ago.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 67: 2470619-66.patch, failed testing. View results

MustangGB’s picture

Issue tags: -Drupal 7.69 target +Drupal 7.70 target
joshmiller’s picture

Status: Needs work » Reviewed & tested by the community

A re-run of the test shows this is working. Back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 67: 2470619-66.patch, failed testing. View results

izmeez’s picture

@joshmiller Are you referring to the patch in comment #49 despite the comments in #58 or are you referring to the patch in #67 ? That one appears to be failing tests on php 7.2 Looks like tests on patch in #67 are passing.

Ronino’s picture

Status: Needs work » Reviewed & tested by the community

Now all tests for #67 succeeded.

I don't get the improvements like in #70, but patch #67 speeds up some of my migrations by 50 %. Great!

MustangGB’s picture

potop’s picture

#67 works fine in our production environment more then 6 months now

mcdruid’s picture

Changes to comments; including fixing a couple of things that didn't quite make sense (e.g. #69) and re-flowing for the line length limit in places.

I'm curious why one of these values is cast to a string in the tests:

    // Make sure we have 2 sample field values that are unique.
    $value1 = 0;
    $value2 = 0;
    while ($value1 == $value2) {
      $value1 = mt_rand();
      $value2 = (string) mt_rand();
    }

...although I don't think it's particularly significant.

I'm also not quite sure what this assertion achieves:

    // Replace the array containing the value with the actual value.
    $entity2->{$this->field_name}[$langcode] = $entity2->{$this->field_name}[$langcode][0];
    $this->assert(_field_sql_storage_write_compare($field_info, $entity1, $entity2), 'The array to hold field values is replaced by the value.');

...but again I don't think it's causing a problem as such.

I've looked at #2297817: Do not attempt field storage write when field content did not change to see if anything else should be backported and not spotted anything obvious.

Leaving at RTBC as I've not changed anything other than comments.

mcdruid’s picture

In #45 @Fabianx suggested:

... it can go in - possibly as opt-in experimental feature first.

This introduces a new variable whereby sites could opt out of this optimization if they need to.

NR for that change.

joseph.olstad’s picture

Status: Needs review » Reviewed & tested by the community

Judging from the interdiff, totally makes sense and looks like similar opt out strategy that was recently put into D7 core by McDruid, since the previous patch was RTBC, I will move this back to RTBC

Fabianx’s picture

RTBC + 1 - looks great, but should be:

- opt in for old sites
- automatic on for new sites and can be disabled

This minimizes risk of data loss and optimizes potential usage.

joseph.olstad’s picture

-1 for the opt in

I think we've already mitigated the risks enough, automated test coverage on all major versions of mysql and even automated tests on sqlite

I'd prefer to set this into the wild and we just deal with whatever comes up like we do with any other major/minor change in core/contrib.

izmeez’s picture

This should be an opt-out. The performance difference has been reported as significant and especially important for migrations. It makes no sense to perpetuate reduced performance as the default.

mcdruid’s picture

As the consequence of this going wrong is data loss, I agree with making it opt-in for existing sites.

We need a Change Record (/ Notice), in which we can extol the performance virtues of this and encourage existing sites to test and opt-in to the optimisation.

joseph.olstad’s picture

Recently the date module release caused data corruption, despite automated tests and years of work on the release, someone reported the issues immediately and it's getting worked on. yes bad stuff can happen but in the name of progress and performance, we should continue to take risks because the payoff in this case is great and we've done a best effort of mitigating the risks.

izmeez’s picture

Comments #70 and #77 may be useful for a Change Record.

joseph.olstad’s picture

Quote from comment #70

to speed up a 72+ hour migration to under an hour on the same hardware.

IMHO, the performance gains are worth the risk, I mean, up to 7200% performance gains, this is more than significant, it is a revolutionary improvement! Just this alone probably merits a fork of Drupal 7 to a Super 7 fork.

just keeps getting better all the time! Great work everyone

pounard’s picture

Glad that after 6 years, it finally gets in, I wish I could go back in time :) I sadly don't maintain any Drupal 7 site anymore, but I sure hope it will help people ! Thanks everybody.

izmeez’s picture

Almost in, keep your fingers crossed.

Fabianx’s picture

RTBC + 1, let's get this in. Thanks for making this opt-in.

For new sites it will be enabled automatically so any new migrations will be able to profit (presuming they are on new sites).

Still needs a Change Record.

joseph.olstad’s picture

On fresh installs this will be enabled by default, for existing sites, it's an opt-in. Oh yes the change record! Hoping someone can do that for us. If someone doesn't do it in March I will do it in April. A little busy atm.

  • mcdruid committed 975e9c6 on 7.x
    Issue #2470619 by mcdruid, pounard, hosef, heddn, DamienMcKenna, boyan....
mcdruid’s picture

Committed (yay!) but still needs the CR, so not marking as Fixed just yet.

joseph.olstad’s picture

I added a draft change record

https://www.drupal.org/node/3205293

izmeez’s picture

@joseph.olstad Thanks for drafting a CR. I have modified it slightly to clarify one line and added a bit on the performance with migrations.

joseph.olstad’s picture

Your CR additions look great, thanks izmeez.

izmeez’s picture

It's good to see this committed. There are a lot of people who made this happen including @mcdruid and @fabianx.

It may be frowned upon but I can't resist putting in a plug for #2994212: field_sql_storage_field_storage_load does use an unnecessary sort in the DB leading to a filesort with a simple patch from @fabianx. I don't have enough knowledge to comment on the SQL but it still applies to the current core and looks like a companion to this issue when it comes to improving performance while not as much as this one.

mcdruid’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record

Thanks for the work on the CR, and thank you to everyone that contributed to this!

mforbes’s picture

I've only skimmed through the comments, so I apologize if this has already been answered. Is there any guidance on how to decide whether an existing site can opt in safely, or is the whole opt-in idea just mitigating the unknown such that safety comes only from waiting for a potential regression issue/release?

MustangGB’s picture

is the whole opt-in idea just mitigating the unknown such that safety comes only from waiting for a potential regression issue/release?

Basically this, I don't believe there are any known issues, that said personally I'd implement it first on a staging/test site copy for a quick check everything is still working as expected.

gbirch’s picture

I'm coming to this very late, but a quick review of _field_sql_storage_write_compare($field, $entity1, $entity2)
makes it appear that the following code

if (empty($entity1->$field_name) && empty($entity2->$field_name)) {
    // Both are empty we can safely assume that it did not change.
    return FALSE;
}

will predictably return the wrong answer when $entity1->$field_name === NULL and $entity2->$field_name == 0 or '0' (or vice versa).

In many cases NULL is really not the same as a zero value, especially in things like questionnaires, where you need to distinguish between a boolean NO (0 by default), and a question that has simply not been answered.

In short, you can't "safely assume" that because two things are empty() they are the same.

Unless of course there is some kind of implicit assumption that field values will always be stored in an array under the field name.

MustangGB’s picture

Unless of course there is some kind of implicit assumption that field values will always be stored in an array under the field name.

If it's not then something else has gone badly wrong, as I understand it this is not the value of the field being checked, just whether the field has has data (array) or has no data (empty).
Presumably empty() is faster than is_array() also.

gbirch’s picture

@MustangGB Yes, you're almost certainly right about that. Sorry for the false alarm.

Phizes’s picture

I'm sorry to have missed the progress of this ticket into core. A major warning for anyone looking to enable this functionality. As it stands, if you change the bundle of an entity, this change is not detected by this code. This will only raise it's head when the entities' old bundle type is deleted, as Drupal cleans up old field values, you will have data loss.

Please opt in with care.

We do use this in production as based on the patch from 67 with an extra check for the entity type. I did not get back to posting a patch as I was having trouble with writing tests for this situation. I can attach a patch without tests later on with the check we have in place for this as we do have a use case where we change the entity type.

While it is an edge case, it was somewhat tricky to pick up on what exactly was going on as there is a delay from the action to the point that the data loss occurs.

In short, you will have data loss if you opt in to the above behaviour in the following scenario:

  1. Create an entity A of bundle1
  2. Create a bundle of type bundle2 (All fields matching bundle1)
  3. Change entity to a type of bundle2
  4. Delete bundle1
  5. Observe data loss on the fields of entity A
MustangGB’s picture

I can attach a patch without tests later

Yes please do! Might make sense to use a new issue though as this is already pretty long.

This does seem like an edge case scenario indeed, I've never heard of anyone changing the bundle of an entity before, but even if it wasn't officially supported I don't see why it should be explicitly broken, and it seems like a pretty reasonable extra check to add.

izmeez’s picture

If you add patch in a different queue please be sure to add a link to the new issue in this issue so others can follow. Thanks.

Phizes’s picture

I've opened a new issue for this, with a new patch, and no tests: https://www.drupal.org/project/drupal/issues/3210388

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.