Problem/Motivation

We need to provide an upgrade path from D7 for the core Text module.

Proposed Resolution

Data in text fields is migrated by #2416765: Migrate Drupal 7 Field/Instance/View mode settings, so this migration only needs to move any of Text's variables into configuration.

Remaining Tasks

  1. Update the patch to match HEAD
  2. Write tests
  3. Review the patch
  4. Commit it
CommentFileSizeAuthor
#33 2409441-33.patch3.07 KBquietone
#28 interdiff-2409441-26-28.txt479 bytesphenaproxima
#28 2409441-28.patch3.1 KBphenaproxima
#26 interdiff-2409441-23-26.txt2.99 KBphenaproxima
#26 2409441-26.patch2.49 KBphenaproxima
#23 interdiff-2409441-22-23.txt2.11 KBquietone
#23 2409441-23.patch3.5 KBquietone
#22 interdiff-2409441-12-22.txt934 bytesquietone
#22 2409441-22.patch2.37 KBquietone
#14 interdiff-2409441-12-14.txt1.06 KBphenaproxima
#14 2409441-14.patch2.56 KBphenaproxima
#12 2409441-12.patch2.37 KBphenaproxima
#8 2409441-8.patch2.36 KBphenaproxima
#6 2409441-6.patch2.35 KBphenaproxima
#5 2409441-5.patch2.64 KBphenaproxima
#1 text_settings-2409441-1.patch619 bytesoadaeh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oadaeh’s picture

oadaeh CreditAttribution: oadaeh commented
Status: Active » Needs work
Issue tags: +Needs tests
FileSize
text_settings-2409441-1.patch619 bytes

Attached is the YAML for this issue, from the patch in #2382117: Migration Files for Drupal 7 Variables.
Test(s) (and maybe a dump file) still need to be written.

miguelc303’s picture

miguelc303 CreditAttribution: miguelc303 at Anexus commented

Added organization support to Anexus IT

jcost’s picture

jcost CreditAttribution: jcost commented
Project: IMP » Drupal core
Version: » 8.0.x-dev
Component: Code » migration system

Will need to be submitted again to Core since moving from sandbox.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Title: Variable to config: text.settings [d7] » Upgrade path for Text 7.x
Priority: Normal » Minor
Issue summary: View changes
Parent issue: #2181257: [meta] Variables to config migration [d7] » #2456259: [META] Drupal 7 to Drupal 8 Migration path
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
FileSize
2409441-5.patch2.64 KB

Updated for HEAD and wrote a test.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Title: Upgrade path for Text 7.x » Migration path for Text 7.x
Issue tags: -Needs tests
FileSize
2409441-6.patch2.35 KB

Re-rolled.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented

Re-rolled in accordance with #2533886: [meta] Move module-specific migration support into the particular modules supported.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
FileSize
2409441-8.patch2.36 KB

Hmm. Having a patch might, just might, help.

chx’s picture

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

o_O that's a config setting? Well, then.

The last submitted patch, 1: text_settings-2409441-1.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work

Needs a reroll...

git ac https://www.drupal.org/files/issues/2409441-8.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2419  100  2419    0     0  25544      0 --:--:-- --:--:-- --:--:-- 25734
error: patch failed: core/modules/migrate_drupal/src/Tests/Table/d7/Variable.php:326
error: core/modules/migrate_drupal/src/Tests/Table/d7/Variable.php: patch does not apply
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
FileSize
2409441-12.patch2.37 KB

Re-rolled. No interdiff due to conflicts in Variable.php dump file.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Needs work

Manually comparing to the D6 test, this is missing config schema testing - it appears the policy is to do this for all configuration migrations: #2293419: Add config schema test to all configuration test in migration, fix bugs.

It would be great if migration paths which don't vary much from the D6 version had diffs against the equivalent D6 files, so we can more easily ensure feature/testing parity.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
FileSize
2409441-14.patch2.56 KB
interdiff-2409441-12-14.txt1.06 KB

Behold!

EDIT: As per the comment below, this one is NOT RTBC. #12 is.

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Murphysboro, IL, USA
CreditAttribution: mikeryan at Acquia commented
Status: Needs review » Reviewed & tested by the community

Well, so sorry you rolled that new patch - looks like we should not bother with schema testing: https://www.drupal.org/node/2293419#comment-10170598

So, RTBC to the #12 patch.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2409441-14.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Related issues: +#2416765: Migrate Drupal 7 Field/Instance/View mode settings

Relating to the fields issue.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Reviewed & tested by the community

#14 failed testing, but #12 did not, and it's the one to be committed. So back to RTBC.

phenaproxima queued 12: 2409441-12.patch for re-testing.

The last submitted patch, 12: 2409441-12.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a re-roll from #12.

quietone’s picture

quietone CreditAttribution: quietone commented
Status: Needs work » Needs review
FileSize
2409441-22.patch2.37 KB
interdiff-2409441-12-22.txt934 bytes

reroll

quietone’s picture

quietone CreditAttribution: quietone commented
FileSize
2409441-23.patch3.5 KB
interdiff-2409441-22-23.txt2.11 KB

The migration templates for d6 and d7 are the same, so merged them.

Status: Needs review » Needs work

The last submitted patch, 23: 2409441-23.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone commented

Why does this fail?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
FileSize
2409441-26.patch2.49 KB
interdiff-2409441-23-26.txt2.99 KB

I'm not sure, but one thing is certain -- when the D6 and D7 versions of a migration are merged, there's no need for two separate tests :) So I have removed the D7 version of the test and it seems OK now...

Status: Needs review » Needs work

The last submitted patch, 26: 2409441-26.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2409441-28.patch3.1 KB
interdiff-2409441-26-28.txt479 bytes

This oughta work.

quietone’s picture

quietone CreditAttribution: quietone commented

@phenaproxima, I missed the change MigrateDrupal6Test.php. And yes, I should have merged the tests.

+1 for RTBC

Kazanir’s picture

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

Looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 2409441-28.patch, failed testing.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +Needs reroll
quietone’s picture

quietone CreditAttribution: quietone commented
Status: Needs work » Needs review
FileSize
2409441-33.patch3.07 KB

And again. No interdiff because it fails. But the change was only to remove one line, loadDumps from the test.

quietone’s picture

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

Back to RTBC

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.0.x. Thanks!

  • webchick committed ac17586 on 8.0.x 
    Issue #2409441 by phenaproxima, quietone, oadaeh, mikeryan: Migration...
lokapujya’s picture

lokapujya
Primary language English
Location Boston
CreditAttribution: lokapujya at Babson College commented
Issue tags: -Needs reroll

Status: Fixed » Closed (fixed)

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