Problem/Motivation

I upgraded from 11.4 to 12.0.0-alpha1 and got repeated failures during text_update_12001().

field.storage.node.body:

id: node.body
field_name: body
entity_type: node
type: text_with_summary
module: text
persist_with_no_fields: true

No content type actually uses this storage in the exported configuration.

The text_with_summary module is not installed.

Steps to reproduce

  1. Install a Drupal 11.4 site.
  2. Upgrade the codebase and Composer dependencies to Drupal 12.0.0-alpha1.
  3. Run drush updatedb.
  4. text_update_12001() fails.
  5. Run updatedb again and it fails again.

Proposed resolution

updatedb should complete successfully.

Remaining tasks

Figure out what is causing this.

AI investigation:

The update hook loads field-storage entities before attempting to install the replacement module:

$storages = \Drupal::entityTypeManager()
  ->getStorage('field_storage_config')
  ->loadByProperties(['type' => 'text_with_summary']);

if ($storages !== []) {
  \Drupal::service('module_installer')->install(['text_with_summary']);
}

Loading the storage calls FieldStorageConfigStorage::mapFromStorageRecords(), which requires the field-type plugin to resolve its class. This throws before execution reaches the module installation.

Issue fork drupal-3621016

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

ptmkenny created an issue. See original summary.

ptmkenny’s picture

ptmkenny commented
Issue summary: View changes

longwave-bot made their first commit to this issue’s fork.

longwave opened merge request !17014

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

We can use entity query which won't actually load the field storage. Also added a test.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Priority: Normal » Major
smustgrave’s picture

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

Seems like a good defense. Marking as don't want any excuse to revert that removal

  • larowlan committed 642babcd on 11.x 
    fix: #3621016 text_update_12001 fails to load text_with_summary

By:...

  • larowlan committed d107690b on main 
    fix: #3621016 text_update_12001 fails to load text_with_summary

By:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed d107690b1ce to main and 642babcd8e1 to 11.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.

  • amateescu committed f20a0fef on 11.x 
    Revert "fix: #3621016 text_update_12001 fails to load text_with_summary...

  • amateescu committed cbef6228 on main 
    Revert "fix: #3621016 text_update_12001 fails to load text_with_summary...
amateescu’s picture

amateescu commented
Version: 11.4.x-dev » main
Status: Fixed » Needs work

The test added here has been breaking every MR pipeline on main with this message:

The update failed with the following message: "Failed: Drupal\Component\Plugin\Exception\PluginNotFoundException: Unable to determine class for field type 'text_with_summary' found in the 'field.storage.node.body' configuration in Drupal\field\FieldStorageConfigStorage->mapFromStorageRecords() (line 167 of /builds/core/modules/field/src/FieldStorageConfigStorage.php)."

Reverted from main and 11.x for now.

catch’s picture

catch
he/him
Primary language English
commented
Priority: Major » Critical

I think this might be failing on main and not 11.x because src/Plugin/Field/FieldType/TextWithSummaryItem.php was removed from 12.x

I think we need to tell people that they have to update to at least 11.5.x before updating to 12.x if they have text_with_summary fields, in which case the update on main would need to be written to rethrow that exception if it runs with a useful message to install text_with_summary module. Or something like that.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +12.0.0 upgrade path

11.x daily pipeline was green for mysqli which tends towards #15 being on the right lines https://git.drupalcode.org/project/drupal/-/pipelines/952544

longwave changed the visibility of the branch 3621016-entity-query-11x to active.

ptmkenny’s picture

ptmkenny commented

I don't know what the "right" fix is to this issue, but in my case, removing field.storage.node.body.yml from the exported config allowed me to update to Drupal 12 from 11.4 because I was not using text_with_summary, but text_with_summary was referenced in that config file.

smustgrave’s picture

smustgrave commented
Related issues: +#3621680: Remove Drupal\Tests\text\Functional\Update\TextWithSummaryUnusedStorageUpdatePathTest as the module no longer exists
smustgrave’s picture

smustgrave commented

what if we just fix the .install and leave the new test out?

smustgrave’s picture

smustgrave commented

What if we update the hook to check if text with summary exists too? If it doesn’t the assumption is someone jumped straight to 12 and could return a message they need to go get the contrib module?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I think the problem is that if another hook fires first and tries to load field definitions first then it will still crash? Also if they have part upgraded will the UI still work for them to enable the module, or are they stuck at that point?

Maybe we need to make the code that crashes more robust in some way, or leave behind a stub plugin or similar?

Not really sure what the right way to go is, just throwing out ideas that I thought of.

smustgrave’s picture

smustgrave commented

If we add a stub like we did migrate_drupal will it still fail as the plugins are gone?

longwave opened merge request !17074

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review

As per #16, MR!17014 is still valid for 11.x, so I reopened that.

MR!17074 is my approach for main - add a requirements hook that refuses to continue if text_with_summary fields exist and the contrib module is not installed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

New test appears to be failing TextWithSummaryUpdatePathTest

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Reviewed & tested by the community

Requirements hook is a good plan and better than my idea to rethrow a more informative exception, moving back to RTBC.

smustgrave’s picture

smustgrave commented

Just happy we didn't need to revert :)

longwave reopened merge request !17014

  • larowlan committed 14611e09 on 11.x 
    fix: #3621016 text_update_12001 fails to load text_with_summary

By:...

  • larowlan committed a3af6101 on main 
    fix: #3621016 text_update_12001 fails to load text_with_summary

By:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed a3af6101cef to main.

Committed and pushed 14611e09455 to 11.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.

Status: Fixed » Closed (fixed)

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