Problem/Motivation

Running the update hooks to go from Group 2.3.1 to Group 3.3.1 deliberately does not convert all group_content_type entities ID to the new format as that could cause all sorts of issues. However, the code in GroupRelationshipTypeStorage::getRelationshipTypeId() does not account for the fact that some of these entities may still be using the old ID pattern.

This leads to errors like the one below:

TypeError: field_ui_entity_operation(): Argument #1 ($entity) must be of type Drupal\Core\Entity\EntityInterface, null given, called in /var/www/html/web/modules/contrib/group/src/Plugin/Group/RelationHandlerDefault/OperationProvider.php on line 78 in field_ui_entity_operation() (line 133 of core/modules/field_ui/field_ui.module).

Steps to reproduce

  1. Clean Drupal install, enable Group 2.3.1
  2. Add a content type with a longer machine name
  3. Enable that node type as a group content type
  4. Export config
  5. composer require drupal/group:^3
  6. drush updb
  7. Export config
  8. Visit the "Set Available Content" page for your group type
  9. Error

Issue fork group-3489243

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

scotwith1t created an issue. See original summary.

pcate’s picture

pcate commented

Doing a test upgrade from v2.3.1 to v3.3.2 I ran into this same error.

pcate’s picture

pcate commented

Any language.content_settings.group_content.* config files were also not updated. Some of those include the ones with hashed ID's so I'm not sure if updating the language config files needs to be part of the update hook to prevent config dependency errors.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Title: Plugins with hashed plugin IDs not converted properly when upgrading from v2 to v3 » Group relationship type IDs with hash not properly updated in v2-3 upgrade path

Updated the fixture and can confirm, will see what I can do.

kristiaanvandeneynde opened merge request !199

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

MR added, now I need to figure out why the IDs weren't adapted. Although I recall being against that because changing a bundle name has all sorts of extra implications. So I think the idea was to keep the old bundle names, but then the code needs to be able to deal with that.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Title: Group relationship type IDs with hash not properly updated in v2-3 upgrade path » The upgrade from v2 to v3 does not change group relationship type IDs but the code does not account for that

Okay, so we're always going to set a group_update_10300_detected_legacy_version state key now and leverage that in GroupRelationshipTypeStorage::getRelationshipTypeId(). This means older sites will be slightly slower as we cannot be sure there what the relationship type ID is.

Will try to take care of this as neatly as I can.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Active » Needs work
Issue tags: +Needs tests

Needs tests for new approach, but basically we now have:

  1. A state key set during the updates that keeps track of whether we came from a legacy version (v1/v2)
  2. A check for said key when trying to form a relationship type ID
  3. If the check passes, we try to load the old ID
  4. If the check fails or no old ID was found, we use the new ID pattern

You can already try the MR out if you wish. I'll only commit it once we have tests, though.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Issue summary: View changes

Updating IS to reflect the real problem, we may need to double-check the upgrade guide and specifically mention that old IDs are fine, even if a bit confusing.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
Issue tags: -Needs tests

Right, added tests to prove this works. Just need to check the update guide now to make sure we got all bases covered.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I've updated the guide here: https://www.drupal.org/docs/extending-drupal/contributed-modules/contrib...

At this point it would be nice if anyone could confirm that the changes here improve the upgrade path, then I can commit it and tag a new release.

pcate’s picture

pcate commented

@kristiaanvandeneynde I just ran a test upgrade from v2 to v3 with this MR and it resolved the error.

pcate’s picture

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

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Priority: Normal » Major
Status: Reviewed & tested by the community » Fixed

Thanks for the steps to reproduce and confirmation. Normally I don't easily assign credit for these types of comments, but given the impact I think quick responses ought to be rewarded.

Status: Fixed » Closed (fixed)

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