Background: Attempted menu links UUID integration in #2353585
Issue #2353585-31: UUID export and import of menu items linking to nodes, terms and users added a commit f260688 into the 7.x-2.x branch. This is not part of any tagged release yet.
The commit does the following:
If the uuid module is disabled, everything works as before.
If the uuid module is enabled:
When exporting a menu link that links to an entity:
- The path and parent path are rewritten, to use a "uuid/(entity type)/(entity uuid)" path instead of the entity path based on entity id.
- If a new identifier is created for the menu link, it will be based on the uuid path, not the original entity path.
When importing a menu link with a uuid path:
- The entity is loaded by uuid, and then the original entity path is restored before saving the menu link into the database.
Problems
The idea itself is not wrong, and I do not find any obvious real bugs in the implementation.
Still this change is risky.
See also #3085880: Revisit UUID handling for menu links.
Problem: Adding complexity to an already fragile system
The 'menu_links' component is already quite fragile as it is, mostly due to changes prior to 7.x-2.11 - see #3075693: Menu link identifier logic is confusing.
The main problem with the 'menu_links' component is that identifiers are generated from data, and changing identifiers for existing menu links is always problematic.
Problem: Missing test cases or informal spec
Without tests or even any informal spec, it is hard to make sure the system works "correctly" in all cases. We don't even know what "correctly" means.
Problem: Noise in existing features
A project with uuid enabled, after updating features, will see menu_links code in exported features changed on features-update.
Problem: Exported code depends on uuid enabled or not
Enabling or disabling the uuid module will change the exported code generated by features-update.
It will even change the identifiers of menu link components.
Problem: UUID is not always the best solution
The idea of UUID paths assumes that entities (e.g. nodes) are synced by UUID between environments.
This is not always the case. Some websites will create entities (nodes) manually on all environments.
In some cases, a better solution could be to use url aliases instead of uuid paths.
Solution
To allow a safe new release, we are going to revert the changes.
This has to be a "re-rolled" revert, because the file has changed since then, mostly due to code style fixes.
Any code style fixes that were part of commit f260688 should not be reverted. Only the functional changes.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | features-7.x-2.x-3162806-3-revert-menu_links-uuid-entity-path.patch | 7.67 KB | donquixote |
Comments
Comment #2
donquixote commentedComment #3
donquixote commentedComment #4
joseph.olstadok, since this was never tagged in a release, shouldn't be too disruptive.
Comment #5
klausiThanks, looks like the correct revert.
Comment #7
donquixote commentedThanks for reviewing!
Comment #8
donquixote commented