Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Steps to reproduce:
- Create a sitemap entity override. For example, go to a node and check "Do not index this entity" option.
- Create a new content type at admin/structure/types/add
- Make sure Do not index entities of type undefined in variant Default option is checked
- Save the content type.
- The
simple_sitemap_entity_overrides
is empty.
The reason for this is a simple_sitemap_entity_form_submit
submit form handler, and the following snippet:
case 'bundle':
$generator->setBundleSettings($f->getEntityTypeId(), $f->getBundleName(), $settings);
if (empty($settings['index'])) {
$generator->removeEntityInstanceSettings($f->getEntityTypeId(), $f->getInstanceId());
}
break;
Since $f->getInstanceId()
is NULL
in case of a bundle, this will remove all entity overrides.
Proposed resolution
I think we should not remove entity overrides in case index is turned off, but if we have to we should limit it to entity IDs that belong to a specific bundle, but not throw away entity overrides that affect other bundles.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#11 | 3156452-removed-overrides-11.patch | 3.04 KB | mbovan |
#2 | 3156452-removed-overrides-2.patch | 2.35 KB | mbovan |
| |||
#2 | 3156452-removed-overrides-2-TEST-ONLY.patch | 1.52 KB | mbovan |
Comments
Comment #2
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI'm attaching a test only patch and a potential fix.
I am not exactly sure why do have the remove instance call, but in case there is a reason for it, I'm limiting the scope of it.
The fix removes the entity instance settings/overrides only on existing bundles (I think there is no point to run a query on a new bundle) and if there are actual overrides that belong to the current bundle.
Comment #3
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedSince this bug results in entity override data loss, I think it's okay to upgrade the priority to Major.
Comment #5
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedI can confirm - apparently it's a regression introduced when we created variants. Not major to me as module loses its own configuration data.
Thanks for the initial patch, will try to find time this weekend to fix the issue.
Comment #6
BerdirYou can set the priority to whatever you want, but this is literally a critical bug according to https://www.drupal.org/node/45111:
> Cause loss/corruption of stored data. (Lost user input, e.g. a failed form submission, is not the same thing as data loss and in most cases is major).
Comment #7
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedHence "to me" - you know, the person who responds to these issues and fixes them within a couple of days most of the time.
But fair enough, thanks for the schooling. Bumping back to major which after reading the definitions seems more appropriate than critical.
Comment #9
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented@mbovan A slightly cleaner approach, please test the dev verison and get back to me.
Comment #10
BerdirI didn't mean it as a "schooling", just pointing out that data is is considered one of the most severe types of bugs.
Anyway, thanks for fixing, can't comment on which approach is cleaner but why ignore the test that we provided?
Comment #11
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedThanks for the updates @gbyte. However, I think we're not done yet, reopening the issue.
Here is a test that confirms the original test scenario was fixed but also proves the bug still exists if you delete a content type.
Comment #12
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedLooking at
Drupal\simple_sitemap\Simplesitemap::setBundleSettings()
:This will also result in removing overrides when you resave bundle settings (for example, disable/enable index).
I think, whenever we call
removeEntityInstanceSettings()
we should only call it if$entity_ids
is not empty. Otherwise, due to its current implementation, it results either in exception or removing unwanted overrides.Comment #14
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented@mbovan Thanks for your keen eye and the tests. Let me know in case something is broken still.
Comment #15
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI can confirm the latest changes fix problems described in #11 and #12.
Also, I manually checked calls where the module deletes overrides and it seems okay now. Thanks!