Follow-up to #2596243: Migrations for data from 7.x-2.x

Problem/Motivation

The related issue at #2596243: Migrations for data from 7.x-2.x has introduced migration for 7.x-2.x. However, that version was not even released and most people are probably using the 1.x version of the module.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#23 newsletter_id_column_not_found--2802227-23.patch392 bytesdom.
#16 define_migrations_for-2802227-16.patch7.56 KBhussainweb
#16 interdiff-14-16.txt1.1 KBhussainweb
#14 define_migrations_for-2802227-14.patch7.1 KBhussainweb
#11 define_migrations_for-2802227-11.patch7.44 KBhussainweb
#11 interdiff-8-11.txt1.01 KBhussainweb
#8 define_migrations_for-2802227-8.patch7.12 KBhussainweb
#7 define_migrations_for-2802227-7.patch6.93 KBhussainweb
#7 interdiff-4-7.txt676 byteshussainweb
#4 define_migrations_for-2802227-4.patch6.83 KBhussainweb
#2 define_migrations_for-2802227-2.patch4.81 KBhussainweb

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new define_migrations_for-2802227-2.patch4.81 KB

Status: Needs review » Needs work

The last submitted patch, 2: define_migrations_for-2802227-2.patch, failed testing.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new define_migrations_for-2802227-4.patch6.83 KB

I forgot a file earlier and there are some more tweaks.

hussainweb’s picture

hussainweb
Primary language English
commented
Title: Define migrations for simplenews subscriber and newsletter data » Define migrations for data from simplenews 7.x-1.x

Status: Needs review » Needs work

The last submitted patch, 4: define_migrations_for-2802227-4.patch, failed testing.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-4-7.txt676 bytes
new define_migrations_for-2802227-7.patch6.93 KB

The error occurs when the migrate module is not enabled. Fixing it...

hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new define_migrations_for-2802227-8.patch7.12 KB

Just some cleanup. I lost the interdiff in creating the patch but I don't think anyone has reviewed this yet.

Status: Needs review » Needs work

The last submitted patch, 8: define_migrations_for-2802227-8.patch, failed testing.

hussainweb’s picture

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

Random failure

hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new interdiff-8-11.txt1.01 KB
new define_migrations_for-2802227-11.patch7.44 KB

I faced an issue with subscribers field not being set correctly. I am attaching the fix.

Status: Needs review » Needs work

The last submitted patch, 11: define_migrations_for-2802227-11.patch, failed testing.

hussainweb’s picture

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

Random failure again.

hussainweb’s picture

hussainweb
Primary language English
commented
StatusFileSize
new define_migrations_for-2802227-14.patch7.1 KB

Rerolling...

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/src/EventSubscriber/MigrationSubscriber.php
@@ -0,0 +1,63 @@
+
+class MigrationSubscriber implements EventSubscriberInterface {
+
+  protected $entityFieldManager;
+
+  public function __construct(EntityFieldManagerInterface $entityFieldManager) {
+    $this->entityFieldManager = $entityFieldManager;
+  }

Missing docs. Can you improve that a bit, then I can commit this.

hussainweb’s picture

hussainweb
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-14-16.txt1.1 KB
new define_migrations_for-2802227-16.patch7.56 KB

Here's some documentation.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Needs review » Reviewed & tested by the community

RTBC per #15

jcnventura’s picture

jcnventura
Primary language English
commented

Yes, indeed. Seems pointless to provide a migration for 2.x when there are 90 (yes nine-zero) sites using that version, while you've got >46000 sites using 1.x, according to https://www.drupal.org/project/usage/simplenews

This should be committed.

  • Berdir committed bb78754 on 8.x-1.x authored by hussainweb 
    Issue #2802227 by hussainweb, Berdir: Define migrations for data from...
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

@jcnventura: That tone seems unnecessarily aggressive. The 7.x-2.x migration was written by us because we needed it. Migration patches are tricky, I can either commit something blindly or have to spend a lot of time to test something myself. It would have been sufficient to just comment that you are successfully using this. Also haven't been doing anything with simplenews in the last year and didn't have any time to maintain it.

Beside, half of the documentation that I requested in #15 was still missing, I added that myself now.

Committed.

jcnventura’s picture

jcnventura
Primary language English
commented

Thanks @berdir. Sorry if the tone sounded aggressive.

And yes, I was suspecting that a 2.x site needed the migration. And I totally get it that you don't have time to maintain everything.

Status: Fixed » Closed (fixed)

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

dom.’s picture

dom. commented
StatusFileSize
new newsletter_id_column_not_found--2802227-23.patch392 bytes

Hi there !
I am using this migration path and got
Column not found: 1054 Unknown column "newsletter_id" in "on clause"
regarding
LEFT OUTER JOIN ...._migrate_simplenews_newsletter map ON newsletter_id = map.sourceid1

This particular LEFT OUTER JOIN is created by SqlBase class and does take the getIds() method keys for field name.
It seems to appear that because newsletter_id is actuallly an alias, this does not work.

For this reason, I add to add the ignore_map: TRUE option to source plugin definition.

Patch attached with this proposition change.

rclemings’s picture

rclemings commented

Am I missing something or does this NOT migrate subscriptions?

I'm able to migrate newsletters, issues, and subscribers, but not the corresponding subscriptions. All of the boxes on /admin/people/simplenews/edit/NNN are unchecked.

Probably not related, but I did have to apply this RTBC core patch to get it to work, possibly because of some orphaned newsletter tids in the source:

https://www.drupal.org/project/drupal/issues/3365895