Problem/Motivation

Spun from:
#3399929: Change wxt_core countries config name

Steps to reproduce

  1. Upgrade to any drupalwxt/wxt commit hash some time between monday and friday the 10th of November
  2. delete the countries yml that was brought in
  3. then later upgrade to the latest drupalwxt/wxt
  4. run update hooks, crash on 8502

Proposed resolution

See patch

Remaining tasks

Review patch

User interface changes

N/A

API changes

update hook 8502 and 8501

Data model changes

N/A

CommentFileSizeAuthor
#10 3401668#10-remove-isNew.patch1.76 KBskaught
#2 3401668-02.patch1.43 KBjoseph.olstad

Comments

joseph.olstad created an issue. See original summary.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Active » Needs review
StatusFileSize
new 3401668-02.patch1.43 KB
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Assigned: joseph.olstad » Unassigned
Parent issue: » #3399929: Change wxt_core countries config name
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Issue summary: View changes
smulvih2’s picture

smulvih2
Location Canada 🍁
commented

@joseph thanks for opening a new ticket for this! Will see if I can reproduce the issue to test this fix.

jmdeleon’s picture

jmdeleon commented

Was getting this same error. Patch in #2 fixes it.

skaught’s picture

skaught
Primary language English
commented
Status: Needs review » Needs work

step 2. delete the countries yml that was brought in

I'm assuming #6 also manually deletes your own config too.

// Update hook 8501 wasn't run correctly for some reason, or the yml was deleted.
in your patch you commented.
also: you do reset 8501 with the new namespace than it was it updated. (ln 678 after patch #2). this changes the condition to see if a user actually changed any config in their site. is this confirmed?

Documentation?
How did you delete the config to begin this test? It's as much a fail point should someone have the ability in an active project to manually delete an active config. We are patching against a dev line if not just from the 5.0.1 release, changing hashes and dropping a config...

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs work » Needs review

@SKAUGHT

An alternative test is this:

Remove the countries yml from your filesystem
import the configs so that it's deleted from your schema.

Now run the following drush commands.

drush eval "\Drupal::service('update.update_hook_registry')->setInstalledVersion('wxt_core', 8501);";
drush updb -y;

doing this without the patch will crash and you'll not be able to upgrade

add the patch and the countries are brought in correctly without crashing.

skaught’s picture

skaught
Primary language English
commented

Your overlaying a rollback situations.. I get that..

- physically removing the config form FS, then reset instance of triggered hook.
I do see this is a complex developer sequence to cause a fail. is this real-world then from the 5.0.1 release. or any sites that upgrade from v4 itself?

In the moment this seems more like {entity}->isNew() doesn't catch something as could be hoped..

skaught’s picture

skaught
Primary language English
commented
StatusFileSize
new 3401668#10-remove-isNew.patch1.76 KB

@joseph.olstad
how does this feel to you sir!

just clean from dev line (I didn't edit from yours)

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Hi @SKAUGHT,
I deleted my countries yml, imported config to make sure the countries were gone then I ran your patch, set the schema version to 8501, ran updb, the end result is that I no longer had any countries.

The countries were not restored.

I prefer patch #2 because it brings back the countries with the expected namespace.

I also re-tested patch #2 , if I have the correct countries value, there's no crashing either when re-running the schema updates with or without expected countries that are brought in (or not) with the new 8501.

skaught’s picture

skaught
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Okay, that sound good to me too!

let move with patch #2.

  • sylus committed 1ccd242a on 5.0.x authored by joseph.olstad 
    Issue #3401668 by joseph.olstad, SKAUGHT: Regression fix for Change...
sylus’s picture

sylus commented
Status: Reviewed & tested by the community » Fixed

Committed and attributed thanks for the fix everyone :D

Status: Fixed » Closed (fixed)

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

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

fixes we made to 5.0.x were not fully merged into 5.1.x

I just got tripped up on this with 5.1.0 , made a new ticket here:

#3409755: 5.1.x requires a merge from wxt 5.0.x - settings (wxt_core countries) fix is missing