Problem/Motivation

Steps to reproduce:

  1. Create a sitemap entity override. For example, go to a node and check "Do not index this entity" option.
  2. Create a new content type at admin/structure/types/add
  3. Make sure Do not index entities of type undefined in variant Default option is checked
  4. Save the content type.
  5. 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mbovan created an issue. See original summary.

mbovan’s picture

I'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.

mbovan’s picture

Priority: Normal » Major

Since this bug results in entity override data loss, I think it's okay to upgrade the priority to Major.

The last submitted patch, 2: 3156452-removed-overrides-2-TEST-ONLY.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

gbyte’s picture

Assigned: Unassigned » gbyte
Priority: Major » Normal
Status: Needs review » Active

I 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.

Berdir’s picture

You 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).

gbyte’s picture

Priority: Normal » Major

You can set the priority to whatever you want, but this is literally a critical bug according to https://www.drupal.org/node/45111

Hence "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.

  • gbyte.co committed be6cb5c on 8.x-3.x
    Issue #3156452 by mbovan, gbyte.co: Updating bundle settings deletes all...
gbyte’s picture

Status: Active » Fixed

@mbovan A slightly cleaner approach, please test the dev verison and get back to me.

Berdir’s picture

I 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?

mbovan’s picture

Status: Fixed » Needs work
FileSize
3.04 KB

Thanks 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.

mbovan’s picture

Looking at Drupal\simple_sitemap\Simplesitemap::setBundleSettings():

      $entity_ids = $this->entityHelper->getEntityInstanceIds($entity_type_id, $bundle_name);

      // Delete all entity overrides in case bundle indexation is disabled.
      if (empty($settings['index'])) {
        $this->removeEntityInstanceSettings($entity_type_id, $entity_ids);

        return $this;
      }

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.

  • gbyte committed 0d766e2 on 8.x-3.x
    Issue #3156452 by mbovan, gbyte: Removing bundle settings deletes all...
  • gbyte committed 8bcc131 on 8.x-3.x authored by mbovan
    Issue #3156452 by mbovan, gbyte: Improve sitemap entity override testing
    
gbyte’s picture

Status: Needs work » Fixed

@mbovan Thanks for your keen eye and the tests. Let me know in case something is broken still.

mbovan’s picture

I 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!

Status: Fixed » Closed (fixed)

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