Support from Acquia helps fund testing for Drupal Acquia logo

Comments

heddn created an issue. See original summary.

Ada Hernandez’s picture

I'm working with @heddn in this new method in paragraphs module and this is the first patch.

Ada Hernandez’s picture

Status: Active » Needs review
heddn’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

And we should also create some tests here. Which I think would require a db fixture. Yuck, but I cannot think of any other way around it. Here's some docs on how to create one: https://www.drupal.org/docs/8/api/migrate-api/generating-database-fixtur...

  1. +++ b/migration_templates/paragraphs_field_collection_type.yml
    @@ -0,0 +1,12 @@
    +  prefix: field_ # DRUPAL_OPTIONAL
    

    If we add a comment, add it on its own line before this. I don't think a comment is necessary though.

  2. +++ b/src/Plugin/migrate/source/FieldCollectionType.php
    @@ -0,0 +1,66 @@
    +      $row->setSourceProperty('id',$name);
    

    Nit: whitespace between comma.

  3. +++ b/src/Plugin/migrate/source/FieldCollectionType.php
    @@ -0,0 +1,66 @@
    +    $row->setSourceProperty('label', ucwords($name));
    

    Comment here that we are doing this because we don't really have a label on D7 field collections. Although we kinda do. It is the field label on the field instance where this field collection is stored. However, the field collection could be re-used in several places. So we don't really have a good bundle name. This seems good enough to me. Although I'd prefer if we remove underscores and replace with spaces and only UC the first word, not all.

Ada Hernandez’s picture

FileSize
1.37 KB
2.26 KB

@heddn thanks., nit fixed

Ada Hernandez’s picture

Status: Needs work » Needs review
heddn’s picture

Status: Needs review » Needs work

While it adds to the repo size, here's why I think a db dump is important. We need to test coming from Field collections and eventually from Paragraphs itself. In both cases, we'll have derivers and other multiple migrations to get the whole thing done correctly. It isn't super huge, but biting the bullet to do an actual migration using real data is the best way to really prove that the whole thing works successfully. We added a fixture to commerce_migrate too and it didn't drastically increase the repo size, so let's just do it.

Start with the latest version of D7, do a minimal install. Then add the latest stable version of field collections and a node with a field collection. Then export the entire db and use it in your test, similar to how core migrate uses it.

+++ b/src/Plugin/migrate/source/FieldCollectionType.php
@@ -35,12 +35,12 @@ class FieldCollectionType extends FieldableEntity {
+    // Set label from bundle because We don't have a label in D7 field collections.

Nit: we should be lowercase and 80 characters.

dhruveshdtripathi’s picture

Status: Needs work » Needs review
FileSize
2.27 KB
683 bytes

Very small changes made suggested in comment #7.

quietone’s picture

Just a few minor things.

  1. +++ b/src/Plugin/migrate/source/FieldCollectionType.php
    @@ -0,0 +1,67 @@
    +    // Remove prefix if it is set in configs.
    ...
    +    $fields = [
    

    This should include 'label' and 'id' as well.

  2. +++ b/src/Plugin/migrate/source/FieldCollectionType.php
    @@ -0,0 +1,67 @@
    +    // Remove prefix if it is set in configs.
    

    s/configs/configration/

  3. +++ b/src/Plugin/migrate/source/FieldCollectionType.php
    @@ -0,0 +1,67 @@
    + // Remove prefix if it is set in configs.

    It would help to have a comment before this to explain the purpose of this block of code. It is not until a few lines down that I learned that it was all to create a label.

Fully agree with have a D7 dump test fixture.

crashtest_’s picture

I wish I would have found this issue before I went and implemented my own migration. Anyhow, not sure if it helps but here is how we did it: https://gist.github.com/teglia/09c64dd27c6a953537d291dcf42f7fc5

davidwhthomas’s picture

Thanks for the work on the field collection to paragraphs migration upgrade path, a great feature to have!

Testing this out, I can migrate the D7 field collection types into paragraph types.
However, the paragraph types aren't populated with the corresponding field collection type fields, so they remain empty.
Am I missing something?

crashtest_’s picture

I created all of my fields first on the D8 site, then saved the configuration with a drush cex. Then it is always ready for the migration.

docans’s picture

Is there a documentation for how to implement this migration

mikelutz’s picture

mikelutz’s picture

I tweaked and rerolled this patch slightly. I changed the query to be against field_config instead of field_config_instance, and moved the files to match with current standards. To be clear, this simply creates paragraphs with the same machine names as the field collections in d7, it doesn't migrate fields, that is a separate migration.

heddn’s picture

Status: Needs review » Needs work

We really need to add a db fixture here. See https://www.drupal.org/docs/8/api/migrate-api/generating-database-fixtur... for some instructions. Basically, take the D7 fixture from core, load it up. Then add a couple field collections, etc. Then export the db and add it to this issue. After that, you can start running migration tests.

mikelutz’s picture

mikelutz’s picture

@heddn I'll work on getting some tests started as part of my work here, and once I've gotten a little farther in this. I'm seeing significant roadblocks wrt multilingual field collections particularly (see https://www.drupal.org/project/paragraphs/issues/2897021#comment-12462297 ). At this point, I'm not sure how much multilingual migrations we can support, so I don't know how complicated to make the fixture db. I have a d7 dev site I'm working off of atm, but give me another couple work days to see how far I can get with everything.

heddn’s picture

Keep the fixture simple for one set of field collections. But go ahead and add multi-lingual as well. Its easier to modify the fixture one or two times and include more than you think you'll need. Rather than nickel and dime things and have to constantly fussing with it.

BTW, thanks for taking this work on. I'm glad to see it progressing again.

mikelutz’s picture

New patch adds a db fixture and tests, along with a second source plugin and migration for d7 paragraph types as well as field collection types.

To anybody looking, please note this is still not a full migration solution for paragraphs, this only creates the paragraph types.Migration of fields and and content are separate migrations and derivatives which I am working on.

docans’s picture

Thanks so much guys for your contributions and efforts. It is highly appreciated

heddn’s picture

+++ b/tests/src/Kernel/ParagraphsMigrationTest.php
@@ -0,0 +1,60 @@
+    $this->assertInstanceOf('Drupal\paragraphs\Entity\ParagraphsType', $bundle);

Use Class::class syntax. It supports refactoring a lot easier.

Can you run a diff against the d7 fixture in this patch and the one provided by d8 core? Reviewing a 2mb fixture is, shall we say, a little tedious. And with that in mind, perhaps we should create a fixture only issue. It will make reviews just that much easier. Can you explain what types of fields, etc you created in the fixture?

mikelutz’s picture

Looking at the diff, I think the act of firing it up in a live instance added more cruft than I had hoped. We could look into trying to create a file manually that could be applied on top of the core fixture just to add the fields and content type we need. I'm not sure if it will be as maintainable, but it might be cleaner.

That being said, I would love it if someone could create a clean fixture to run tests off of somewhere. It would help me write tests as I go. I'm currently being sponsored to get a migration working for a client, but I don't have the hours budgeted to focus on things like the fixture at the moment. I'm just trying to get the migrations to a point where they can be usable.

In the mean time, the one included created a new content type:
Paragraphs Migration Test (Machine name: paragraphs_migration_test)
containing fields field_paragraph_test and field_field_collection_test

A paragraph bundle:
Paragraph Bundle Test (paragraph_bundle_test)
containing existing fields field_text and field_email

A field collection field:
field_collection_test containing existing fields field_text and field_text plain

and 2 pieces of content, one in an undefined language and one with english and icelandic translations.

Title: Paragraphs Migration Test Single Language
Language: Undefined
Field Collection Test:
Field Collection Text:
Field Collection Text Data
Field Collection Text plain: Field Collection Text Plain Data

Paragraph Test:
Text:
Email:
joe@joe.com
jeff@jeff.com
Text: Paragraph Text two
Email: John@john.com

Title: Paragraphs Migration Test Translatable English
Language: English
Field Collection Test:
Field Collection Text:
Field Collection Text Data English
Field Collection Text plain: Field Collection Text Plain Data
Paragraph Test:
Text:
Paragraph Text English
Email: joe@joe.com

Title: Paragraphs Migration Test Translatable Icelandic
Primary tabs
Language: Icelandic
Field Collection Test:
Field Collection Text:
Field Collection Text Data Icelandic
Field Collection Text plain: Field Collection Text Plain Data Icelandic

Paragraph Test:
Text:
Paragraph Text Icelandic 1
Email: Joe@jo.is
Text: Paragraph Text Icelandic 2
Email: jeff@jeff.is

heddn’s picture

Issue tags: -Needs tests

If we make the single nit change from #22, I'm good with RTBC. We can iterate on this thing in follow-ups. The code in question is very grockable and works. As proved by the tests. And your description of the created fields is good enough for me. If its wrong or not complete, we can fix it in follow-ups. Removed tag, since we now have tests.

Having a dedicated fixture is ugly but is stable and works very well to build tests. Anything else is very fragile.

mikelutz’s picture

heddn’s picture

Status: Needs work » Needs review

Don't feel like you have to do everything here. #2897021: [META] Migrate support for importing field collections as paragraphs has a whole plan of attack.

mikelutz’s picture

I know, I'm working through it. I'll post the patches in the right issues, although they will all end up depending on each other.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me.

mikelutz’s picture

I fixed an issue with removing the prefix 'field_' from field collection bundles when turning them into paragraphs. The original str_replace function was removing all instances of field_, turning 'field_field_collection_test' into 'collection_test' when it should have been 'field_collection_test' I missed this when writing the test, so I updated that as well.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

I see what was happening. field_field_collection_test was getting shortened to collection_test. Too greedy. Great fix. Back to RTBC.

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

Looks very nice, thank you for bringing this to Paragraphs out of the box.

Quickly discussed if it makes sense to import both on Setup and then have two test methods, but the overhead seems to be not really relevant compared to our total test volume.
Committed as-is.

noel.delacruz’s picture

Hi, would like to ask if there is any documentation to implementing this migration. I've managed to apply the patch and migrated the collection types to paragraph types. Unfortunately, the paragraph types are not populated with the fields that I've added on my field collection. Is there something else that I need to do or do I need to manually add them?

mikelutz’s picture

Status: Fixed » Closed (fixed)

The patch in this issue has already been included in the latest -dev for paragraphs, and was only designed to create the paragraph types, not to migrate and fields or content. The work for that is very much ongoing, and in progress.

If you would like to try the latest solution which includes fields and content, install the latest -dev and try the patch from https://www.drupal.org/project/paragraphs/issues/2897021 . Just be aware it is a work in progress and may or may not work for you.

colan’s picture

#34: It's better to leave issues as Fixed in case there's any follow-up discussion to be had. After two weeks with no activity, the system will automatically close. Closing manually removes from everyone's radar right away, which inhibits this discussion, and prevents anyone except maintainers from reopening.