Problem/Motivation

See #2594425-137: Add the option in system menu block to "Expand all items in this tree"

This issue broke the upgrade test for DER 1.x to 2.x https://www.drupal.org/pift-ci-job/1147416.

Proposed resolution

Add block module check or move the post-update hook to block module

Remaining tasks

  • Finalize the approach.
  • Add more upgrade path test.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#15 3020718-15.patch34.65 KBjibran
#15 screenshot-127.0.0.1-2018.12.24-11-51-00.png47.62 KBjibran
#15 interdiff-c0f57f.txt728 bytesjibran
#13 3020718-13.patch33.85 KBjibran
#13 interdiff-479938.txt728 bytesjibran
#12 screenshot-127.0.0.1-2018.12.23-15-44-08.png150.12 KBjibran
#5 3020718-5.patch33.85 KBSam152
#5 3020718-5_TEST-ONLY.patch33.14 KBSam152
2594425-follow-up-138.patch730 bytesjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran created an issue. See original summary.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Good catch, I thought "block" may have been one of those required modules like "user" that core doesn't work without, but installing from "testing", it is indeed disabled.

I'd vote to keep the hook in system, so the name of the function does't have to change for folks who may have already started using the patch.

I suppose testing this would involve creating a new DB fixture, probably from the "testing" profile, and simply running running updb against it. That would probably help catch dependency issues in the future, so worth doing IMO.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Needs upgrade path tests

DER install minimum so I think minimum DB dump should work.

Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

Minimal contains block as a dependency:

name: Minimal
type: profile
description: 'Build a custom site without pre-configured functionality. Suitable for advanced users.'
version: VERSION
core: 8.x
install:
  - node
  - block
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented
Status: Needs work » Needs review
Issue tags: -Needs tests, -Needs upgrade path tests
FileSize
3020718-5_TEST-ONLY.patch33.14 KB
3020718-5.patch33.85 KB

The last submitted patch, 5: 3020718-5_TEST-ONLY.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Minimal contains block as a dependency

Well that is surprising because I generated DER DB from Minimal.

EDIT: I lied it was indeed created from testing profile. Please ignore my holiday mind.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks, for the patch and tests.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: system_post_update_add_expand_all_items_key_in_system_menu_block fails if blocks module is disable » system_post_update_add_expand_all_items_key_in_system_menu_block fails if blocks module is disabled

🤓

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Issue summary: View changes

Thanks, 😂

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/modules/system/system.post_update.php
--- /dev/null
+++ b/core/modules/system/tests/fixtures/update/drupal-8.0.0.bare.testing.php.gz

Is this really built on 8.0.0?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Issue summary: View changes
FileSize
screenshot-127.0.0.1-2018.12.23-15-44-08.png150.12 KB

Seems like 8.1.0

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff-479938.txt728 bytes
3020718-13.patch33.85 KB

Changed the DB name.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs review

Well we jump from system_update_8014 to system_update_8200 and I think system_update_8014() was prior to 8.0.0 so ... but I think maybe we should have a more up-to-date db. something like 8.6.0 instead.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Issue summary: View changes
FileSize
interdiff-c0f57f.txt728 bytes
screenshot-127.0.0.1-2018.12.24-11-51-00.png47.62 KB
3020718-15.patch34.65 KB

Here we go. I excluded DB from interdiff.

shahzad-anwar’s picture

shahzad-anwar CreditAttribution: shahzad-anwar as a volunteer and at Devtron Genesis commented
Status: Needs review » Reviewed & tested by the community

As #14 has been addressed, so setting it back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed 48eb105 and pushed to 8.7.x. Thanks!

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented

Thanks, DER is green again https://www.drupal.org/pift-ci-job/1159989.

Status: Fixed » Closed (fixed)

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