Postponed on #2920662: Fix attribute dependencies and field settings

When migrating some of the attributes I have in a U6 site, I get a field storage error:

Attempt to create a field storage with an name longer than 32 characters: attribute_upgrade_full_day_pilot_training_6h (core/modules/field/src/Entity/FieldStorageConfig.php:316)

It seems that the attributes in U6 didn't have any limit and were auto generated on the attribute title. Is there anything we can do to ensure that they are kept below a string length? Could we maybe test the length and if its too long then use the attribute ID instead?

CommentFileSizeAuthor
#30 2904212-30.patch12.87 KBquietone
#24 interdiff.txt516 bytesquietone
#24 2904212-24.patch8.13 KBquietone
#22 interdiff.txt4.29 KBquietone
#22 2904212-22.patch7.94 KBquietone
#20 2904212-20.patch3.46 KBquietone
#17 2904212-17.patch2.66 KBwilleaton
#15 2904212-15.patch2.52 KBwilleaton
#14 2904212-14.patch5.72 KBquietone
#14 interdiff.txt3.24 KBquietone
#5 2904212-2.patch2.53 KBwilleaton
#3 2904212-1.patch2.74 KBwilleaton

Comments

willeaton created an issue. See original summary.

heddn’s picture

heddn
Primary language English
Location Illinois
commented

Make sure the attribute name follows the machine name format rules, cut the length, then dedupe it. make sure when cutting the length that the dedupe process doesn't make the length go over 32 characters again.

https://www.drupal.org/docs/8/api/migrate-api/migrate-process/process-pl...
https://www.drupal.org/docs/8/api/migrate-api/migrate-process-plugins/pr...
https://www.drupal.org/docs/8/api/migrate-api/migrate-process/process-pl...

willeaton’s picture

willeaton commented
Status: Active » Needs review
StatusFileSize
new 2904212-1.patch2.74 KB

Patch fixing this attached. Name/ID/machine_name is limited to 20 characters because things like "attribute_" is added as a prefix and dedup can add stuff at the end

Status: Needs review » Needs work

The last submitted patch, 3: 2904212-1.patch, failed testing. View results

willeaton’s picture

willeaton commented
StatusFileSize
new 2904212-2.patch2.53 KB

Patch failed because I was deduping things that are ok to be duplicates. Adding new patch file

willeaton’s picture

willeaton commented
Status: Needs work » Needs review
willeaton’s picture

willeaton commented

Can someone help? Is this an error because the data doesn't import properly now or because there is something wrong with the test?

quietone’s picture

quietone commented
Status: Needs review » Needs work

Patch looks good, just one question.

Please remind me why the attribute id in d6_ubercart_product_attribute can be a duplicate.

willeaton’s picture

willeaton commented

In the D8 model the attribute is duplicated for every attribute value I think. I realised this when the first patch failed the tests (see patch 1 test results). As I can't migrate products or nodes atm with the other patches I have applied I'm not sure. Can someone from D8 side check it's fine?

willeaton’s picture

willeaton commented
Status: Needs work » Needs review
heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Needs work
Issue tags: +Needs tests 
  1. --- a/modules/ubercart/migration_templates/d6_ubercart_attribute_value.yml
+++ b/modules/ubercart/migration_templates/d6_ubercart_attribute_value.yml

    This looks good. Values can be duplicated. 

  2. +++ b/modules/ubercart/migration_templates/d6_ubercart_field_attribute.yml
--- a/modules/ubercart/migration_templates/d6_ubercart_product_attribute.yml
+++ b/modules/ubercart/migration_templates/d6_ubercart_product_attribute.yml

    Does this need to map up with d6_ubercart_field_attribute? Can/should we have a test for this?

willeaton’s picture

willeaton commented

Honestly, I don't know, this is where a D8 commerce person needs to evaluate. I believe
/modules/ubercart/migration_templates/d6_ubercart_field_attribute.yml
had to be shorter (string limit 20) because it prefixes with "attribute_". Therefore it doesn't look like they need to match although it would make sense...

quietone’s picture

quietone commented 
  1. +++ b/modules/ubercart/migration_templates/d6_ubercart_field_attribute.yml
@@ -15,8 +15,16 @@ process:
+      plugin: dedupe_entity

    The is deduping the d6 field name, should dedupe the d8 storage name, field_name.

    And add a comment as to why the length is 20. That will catch some up in the future. 

  2. +++ b/modules/ubercart/migration_templates/d6_ubercart_product_attribute.yml
@@ -6,23 +6,27 @@ migration_tags:
\ No newline at end of file

    Needs a new line.

As for the tests, adding an attribute with a long name to the test fixture should provide the data.

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.24 KB
new 2904212-14.patch5.72 KB

Went back to the beginning to see what D8 does what creating a product attribute. When an attribute is created through the UI in D8, both field storage config and field instance config are created. The latter does not have a migration.

After using drush to migrate d6_ubercart_product_attribute and d6_ubercart_product_attribute the atatirbutes and not the field config the attributes seem to work fine through the UI. Don't fully understand how about fields and storage and instances. I would think something would break if the config and instances wasn't created. Or, I didn't test something that would fail.

Then I added a test for the problem at hand, the names > 32 character. For that I looked at the taxonomy vocabulary migration and used the way the deduping is done there. That all seems to work now.

Still to do is to review the field storage migration and to create a field instance migration.

willeaton’s picture

willeaton commented
StatusFileSize
new 2904212-15.patch2.52 KB

Rerolled the patch as it failed to apply with the new alpha2 version. I completely removed the changes to tests and uc6 fixture as this only created complicated conflicts. Need to add them back in.

As per quietone's last comment...
Still to do is to review the field storage migration and to create a field instance migration.

willeaton’s picture

willeaton commented
Status: Needs review » Needs work

Hi all, this seems to have gone into alpha2 which fixed one issue (error on migrate due to attribute names being too long for D8 when coming from D6). The new issue is that we've broken the attribute relationship in D8 and after doing a full migration I get a great big error...

PHP Fatal error: Call to a member function getSetting() on null in web/modules/contrib/commerce/modules/product/commerce_product.views.inc on line 28

Whats happening is that the field storage bundle is empty because (I think) the attribute table its looking for doesn't exist.

We have 3 migrations:
d6_ubercart_attribute_value.yml
d6_ubercart_product_attribute.yml
d6_ubercart_field_attribute.yml

and the attribute name/id/machine name isn't migrated in the same way in either

willeaton’s picture

willeaton commented
StatusFileSize
new 2904212-17.patch2.66 KB

OK, I've learned from my mistakes and I now have attributes reduced to a max length, deduped, AND now they match between the 3 migrations doing a migration_lookup. Basically I have a d6 to d8 migration working with attributes. Patch attached (based on alpha2).

New problem, the attributes don't seem to be associated to the products in D8 and Im not sure where in the D8 database an attribute is associated to a product. Can someone please tell me what table? @mglaman?

willeaton’s picture

willeaton commented
Status: Needs work » Needs review

Setting to NR for testbot

Status: Needs review » Needs work

The last submitted patch, 17: 2904212-17.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2904212-20.patch3.46 KB

Change an existing attribute name to be longer than 32 characters.

Status: Needs review » Needs work

The last submitted patch, 20: 2904212-20.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Needs review
StatusFileSize
new 2904212-22.patch7.94 KB
new interdiff.txt4.29 KB

Change tests to use the new longer attribute name. Modified the input to assertProductAttributeEntity to be simpler. And some comment changes.

Status: Needs review » Needs work

The last submitted patch, 22: 2904212-22.patch, failed testing. View results

quietone’s picture

quietone commented
Status: Needs work » Postponed
StatusFileSize
new 2904212-24.patch8.13 KB
new interdiff.txt516 bytes

With #2915878: Call to a member function getSetting() on null this patch installed locally the failing test passes.

Now postponing on #2915878: Call to a member function getSetting() on null .

quietone’s picture

quietone commented
Status: Postponed » Needs work

No longer postponed

quietone’s picture

quietone commented
Status: Needs work » Needs review

Testing

quietone’s picture

quietone commented
Issue summary: View changes
Status: Needs review » Postponed

This will need a reroll after #2920662: Fix attribute dependencies and field settings is committed. Let's wait on that.

willeaton’s picture

willeaton commented

With the latest patch, the attributes dont seem to get set up.
I see this error:

Attempt to create a field attribute_yate_exclusivo_baja_al that does not exist on entity type commerce_product_variation

I then also get the old error:
Call to a member function getSetting() on null in /web/modules/contrib/commerce/modules/product/commerce_product.views.inc on line 28

This used to happen before when the attributes didn't migrate properly. Something has stopped working...

quietone’s picture

quietone commented
Status: Postponed » Active
Issue tags: +Needs reroll

No longer postponed.

quietone’s picture

quietone commented
Status: Active » Needs review
Issue tags: -Needs tests, -Needs reroll
StatusFileSize
new 2904212-30.patch12.87 KB

All the attribute migrations now use migration_lookup to get the field name from d6_ubercart_attribute_field. Tests updated as required for the attribute with a name longer than 32 characters.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me. Almost entirely yml file changes. With a few tests. So, LGTM.

heddn’s picture

heddn
Primary language English
Location Illinois
commented
Status: Reviewed & tested by the community » Fixed

  • heddn committed 92865b0 on 8.x-2.x authored by quietone 
    Issue #2904212 by quietone, willeaton, heddn: [Ubercart D6] attribute...

Status: Fixed » Closed (fixed)

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