Problem/Motivation

Because d7_shortcut has a required dependency on d7_menu_links, it ends up creating a circular dependency if #3051251: Existing menu links show validation issues on migration (and ALL menu links pointing to node translations are invalid) is also applied. (That issue's patch generates derivatives for d7_menu_links, for example d7_menu_links:node:article for all menu links pointing to article nodes.)

Steps to reproduce

  1. Apply #3051251: Existing menu links show validation issues on migration (and ALL menu links pointing to node translations are invalid)
  2. Migrate with a source site that has at least one menu link pointing to a node.

Proposed resolution

Make d7_menu_links an optional instead of a required dependency of d7_shortcut.

As explained in #7, the dependency on the d7_menu_links migration is safe to completely remove.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#11 interdiff_7_11.txt1.47 KBanmolgoyal74
#11 3165944-11.patch2.36 KBanmolgoyal74
#7 3165944-7.patch460 byteswim leers
#2 3165944-2.patch474 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 3165944-2.patch474 bytes

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

tanubansal’s picture

tanubansal commented

Tested #2 on 9.1, d7_menu_links is an optional now

quietone’s picture

quietone commented
Component: shortcut.module » migration system

This will get more attention in migration system component

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

🙏

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2500513: Upgrade path for Shortcut 7.x, +#2669978: Migrate D7 Menu Links, +#2021779: Decouple shortcuts from menu links
StatusFileSize
new 3165944-7.patch460 bytes

In fact … I don't see why we need this dependency at all.

It was introduced in #2500513: Upgrade path for Shortcut 7.x (back then still menu_links, #2669978: Migrate D7 Menu Links renamed the menu_links migration to d7_menu_links.

Because as of #2021779: Decouple shortcuts from menu links, shortcuts are independent of menu links. Yes, during the migration, we need to query the Drupal 7 menu_links table, but there's nothing in the d7_menu_links that the migration of shortcuts depends on!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: d7_shortcut should not have a required dependency on d7_menu_links » d7_shortcut should not have a dependency on d7_menu_links
Issue summary: View changes

Updating issue title & summary per #7.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: d7_shortcut should not have a dependency on d7_menu_links » d7_shortcut migration should not have a dependency on d7_menu_links
quietone’s picture

quietone commented
Status: Needs review » Needs work

The migration kernel tests for shortcut need to be updated to remove the running of d7_menu_links.yml. That should provide the proof that the dependency is not necessary.

anmolgoyal74’s picture

anmolgoyal74 commented
Status: Needs work » Needs review
StatusFileSize
new 3165944-11.patch2.36 KB
new interdiff_7_11.txt1.47 KB
quietone’s picture

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

This looks fine.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed ca638fa1e9 to 9.2.x and 995eb1c011 to 9.1.x. Thanks!

  • alexpott committed ca638fa on 9.2.x 
    Issue #3165944 by Wim Leers, anmolgoyal74, quietone: d7_shortcut...

  • alexpott committed 995eb1c on 9.1.x 
    Issue #3165944 by Wim Leers, anmolgoyal74, quietone: d7_shortcut...

Status: Fixed » Closed (fixed)

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