Problem/Motivation

In the #2907780: Add a field purger service commits contain errors.

The config name and its key were misnamed in the commit.

This code in SystemHooks::cron()

    // Do a pass of purging on deleted entity field data, if any exists.
    if (!$config_purge_batch_size = \Drupal::config('field.field_settings')->get('purge_batch_size')) {
      $config_purge_batch_size = Settings::get('field_purge_batch_size', 50);
    }
    \Drupal::service(FieldPurger::class)->purgeBatch($config_purge_batch_size);

doesn't read a customized config value anymore, because of the incorrect config name - field.field_settings instead of field.settings. The same situation in ConfigImporterFieldPurger::initializeSandbox().

The config key purge_batch_size is incorrectly referenced as field_purge_batch_size in the return string of field_post_update_clear_purge_batch_size().

Steps to reproduce

Proposed resolution

Fix names of the config and its key.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3622469

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pingwin4eg created an issue. See original summary.

hoxton made their first commit to this issue’s fork.

hoxton’s picture

hoxton commented

tried looking for other places were this issue could have been present, found non.
have raised PR for current issue.

hoxton’s picture

hoxton commented
Status: Active » Needs review

hoxton opened merge request !17090

leandro713’s picture

leandro713 commented

I may be missing something, but I noticed two points while reviewing MR !17090.

The change from field.field_settings to field.settings looks correct. However, the MR also changes the fallback in ConfigImporterFieldPurger::initializeSandbox() from:

Settings::get('field_purge_batch_size', 50)

to:

Settings::get('purge_batch_size', 50)

As I understand it, purge_batch_size is the key inside the field.settings configuration object, while field_purge_batch_size is the corresponding key in settings.php. The existing post-update function and SystemHooks::cron() also use Settings::get('field_purge_batch_size', 50).

The issue summary also mentions correcting the returned message in field_post_update_clear_purge_batch_size(), from field.settings:field_purge_batch_size to field.settings:purge_batch_size, but field.post_update.php does not appear to be changed in the current MR.

Am I reading this correctly?

pingwin4eg’s picture

pingwin4eg
Location Zaporizhia 🇺🇦
commented
Status: Needs review » Needs work

Right. The setting is a new one (if I understand correctly from the original issue), so don't change it.

The issue summary lists all places with errors. Don't look further.

ATM I can't say whether anything else should be done regarding the fact that the wrong config landed in a stable release.

hoxton’s picture

hoxton commented

thanks @leandro713 for pointing out the needed corrections. Have corrected them likewise.

leandro713’s picture

leandro713 commented
Status: Needs work » Needs review

Thanks @hoxton. I reviewed the updated diff and the three affected locations now look consistent. The changes address the points I raised, and the pipeline is green.

oily changed the visibility of the branch 3622469-fix-field.settingspurgebatchsize-config to hidden.

ironnuts’s picture

ironnuts commented

This being a bug test coverage is required. Perhaps it can be added to core/modules/field/tests/src/Kernel/BulkDeleteTest::testPurgeField.

ironnuts’s picture

ironnuts commented
Issue tags: +Needs test
ironnuts’s picture

ironnuts commented

If this is simply a case of wrong naming then possibly no test required. But there is the question why existing test coverage is not detecting the bug. Ok, will set to RTBTC. If anyone thinks test is needed they can flip back to Needs work.

ironnuts’s picture

ironnuts commented
Issue tags: -Needs test
ironnuts’s picture

ironnuts commented
Status: Needs review » Reviewed & tested by the community

  • longwave committed 46285c7a on 11.4.x 
    fix: #3622469 Fix field.settings:purge_batch_size config name and key...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Debated whether to backport to 11.4.x as it applies there, but it touches a translatable string. However the wording (and code) is incorrect so I think we should fix it there as well. We also have entity_update_batch_size already so I wonder why we need a different setting for this, it feels like this should only need one value really. But that's for another issue to argue!

Committed and pushed to main, 12.0.x, 11.x, 11.5.x and 11.4.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • longwave committed 3f4dcf97 on 11.5.x 
    fix: #3622469 Fix field.settings:purge_batch_size config name and key...

  • longwave committed cb512a9e on 11.x 
    fix: #3622469 Fix field.settings:purge_batch_size config name and key...

  • longwave committed 355a92b7 on 12.0.x 
    fix: #3622469 Fix field.settings:purge_batch_size config name and key...

  • longwave committed f2abd414 on main 
    fix: #3622469 Fix field.settings:purge_batch_size config name and key...