#318 | 2447727-318.patch | 25.38 KB | quietone |
|
#318 | diff-315-318.txt | 1.7 KB | quietone |
#315 | 2447727-315.patch | 25.38 KB | quietone |
|
#315 | diff-311-315.txt | 764 bytes | quietone |
#313 | diff-311-313.txt | 1002 bytes | quietone |
#313 | 2447727-313.patch | 25.15 KB | quietone |
|
#311 | 2447727-311.patch | 25.35 KB | quietone |
|
#311 | interdiff-309-311.txt | 631 bytes | quietone |
#309 | 2447727-309.patch | 25.34 KB | quietone |
|
#309 | interdiff-308-309.txt | 636 bytes | quietone |
#308 | 2447727-308.patch | 25.33 KB | quietone |
|
#308 | interdiff-304-306.txt | 3.25 KB | quietone |
#306 | 2447727-306.patch | 25.17 KB | quietone |
|
#306 | interdiff-303-306.txt | 2.77 KB | quietone |
#303 | interdiff-2447727-293-303.txt | 1.3 KB | mradcliffe |
#303 | 2447727-303.patch | 22.42 KB | mradcliffe |
|
#293 | 2447727-293.patch | 22.44 KB | quietone |
|
#293 | interdiff-290-293.txt | 905 bytes | quietone |
#290 | 2447727-290.patch | 22.45 KB | quietone |
|
#290 | interdiff-286-290.txt | 536 bytes | quietone |
#286 | interdiff_282-286.txt | 1.18 KB | Neslee Canil Pinto |
#286 | 2447727-286.patch | 22.45 KB | Neslee Canil Pinto |
|
#282 | interdiff_279-282.txt | 1.19 KB | Neslee Canil Pinto |
#282 | 2447727-282.patch | 23.4 KB | Neslee Canil Pinto |
|
#279 | interdiff_276-279.txt | 2.4 KB | Neslee Canil Pinto |
#279 | 2447727-279.patch | 23.43 KB | Neslee Canil Pinto |
|
#276 | interdiff_274-276.txt | 2.18 KB | Neslee Canil Pinto |
#276 | 2447727-276.patch | 25.06 KB | Neslee Canil Pinto |
|
#274 | interdiff_271-274.txt | 2.28 KB | Neslee Canil Pinto |
#274 | 2447727-274.patch | 25.15 KB | Neslee Canil Pinto |
|
#272 | interdiff-2447727-270-271.txt | 4.6 KB | mradcliffe |
#272 | 2447727-271.patch | 25.16 KB | mradcliffe |
|
#270 | 2447727-270.patch | 25.72 KB | mradcliffe |
|
#270 | interdiff-2447727-269-270.txt | 2.99 KB | mradcliffe |
#269 | 2447727-269.patch | 24.45 KB | mradcliffe |
|
#269 | interdiff-2447727-267-269.txt | 3.59 KB | mradcliffe |
#267 | 2447727-267.patch | 22.44 KB | mradcliffe |
|
#259 | 2447727-259.patch | 25.2 KB | quietone |
|
#259 | interdiff-257-259.txt | 1.75 KB | quietone |
#257 | interdiff-256-257.txt | 13.78 KB | quietone |
#257 | 2447727-257.patch | 26.35 KB | quietone |
|
#256 | 2447727-256.patch | 29.64 KB | quietone |
|
#254 | 2447727-254.patch | 29.64 KB | quietone |
|
#254 | interdiff-252-254.txt | 538 bytes | quietone |
#252 | 2447727-252.patch | 28.95 KB | quietone |
|
#252 | interdiff-250-252.txt | 31.86 KB | quietone |
#250 | 2447727-250.patch | 64.61 KB | quietone |
|
#246 | interdiff-2447727-244-246.txt | 667 bytes | yogeshmpawar |
#246 | 2447727-246.patch | 62.87 KB | yogeshmpawar |
|
#244 | interdiff-2447727-242-244.txt | 674 bytes | yogeshmpawar |
#244 | 2447727-244.patch | 62.87 KB | yogeshmpawar |
|
#242 | 2447727-242.patch | 62.87 KB | yogeshmpawar |
|
#240 | 2447727-240.patch | 62.84 KB | jofitz |
|
#240 | interdiff-38-40.txt | 701 bytes | jofitz |
#238 | 2447727-238.patch | 62.83 KB | jofitz |
|
#236 | 2447727-236-D8.patch | 55.43 KB | mohit1604 |
|
#233 | interdiff.txt | 2.12 KB | quietone |
#233 | 2447727-233.patch | 62.86 KB | quietone |
|
#232 | 2447727-232.patch | 63.47 KB | jofitz |
|
#232 | interdiff-230-232.txt | 668 bytes | jofitz |
#230 | 2447727-230.patch | 63.47 KB | jofitz |
|
#230 | interdiff-228-230.txt | 15.44 KB | jofitz |
#228 | 2447727-228.patch | 66.86 KB | jofitz |
|
#226 | 2447727-226.patch | 66.81 KB | jofitz |
|
#226 | interdiff-224-226.txt | 9.63 KB | jofitz |
#224 | 2447727-224.patch | 63.94 KB | jofitz |
|
#224 | interdiff-222-224.txt | 21.19 KB | jofitz |
#222 | add_migrate_support_for-2447727-222.patch | 60.18 KB | quietone |
|
#217 | 2447727-213-reroll-217.patch | 60.67 KB | joelpittet |
|
#213 | add_migrate_support_for-2447727-213.patch | 61.23 KB | jofitz |
|
#213 | interdiff-211-213.txt | 1.73 KB | jofitz |
#211 | add_migrate_support_for-2447727-211.patch | 61.25 KB | jofitz |
|
#211 | interdiff-209-211.txt | 2 KB | jofitz |
#209 | add_migrate_support_for-2447727-209.patch | 61.41 KB | jofitz |
|
#204 | add_migrate_support_for-2447727-204.patch | 60.95 KB | jofitz |
|
#191 | add_migrate_support_for-2447727-191.patch | 61.99 KB | dimaro |
|
#191 | interdiff-2447727-188-191.txt | 1.43 KB | dimaro |
#188 | 2447727-188.patch | 61.87 KB | iMiksu |
|
#188 | interdiff.txt | 6.23 KB | iMiksu |
#184 | 2447727-184.patch | 61.5 KB | keithm |
|
#182 | 2447727-182.patch | 61.51 KB | hussainweb |
|
#178 | 2447727-178.patch | 61.85 KB | hussainweb |
|
#178 | interdiff-176-178.txt | 14.53 KB | hussainweb |
#176 | 2447727-175.patch | 63.06 KB | hussainweb |
|
#167 | 2447727-167.patch | 63.05 KB | webflo |
|
#165 | 2447727-164.patch | 63.97 KB | webflo |
|
#164 | 2447727-164.patch | 3.44 KB | webflo |
|
#162 | 2447727-162.patch | 63.85 KB | webflo |
|
#160 | 2447727-160.patch | 63.19 KB | webflo |
|
#158 | 2447727-155.patch | 63.15 KB | webflo |
|
#158 | 2447727-158.interdiff.txt | 2.34 KB | webflo |
#155 | 2447727-155.patch | 63.15 KB | webflo |
|
#154 | 2447727-154.patch | 62.32 KB | webflo |
|
#152 | add_migrate_support_for-2447727-152.patch | 64.11 KB | dimaro |
|
#147 | 2447727-147.patch | 64.01 KB | tlyngej |
|
#136 | 2447727-135.patch | 64.01 KB | webflo |
|
#136 | 2447727-135.interdiff.txt | 751 bytes | webflo |
#133 | 2447727-133.interdiff.txt | 2.98 KB | webflo |
#133 | 2447727-133.patch | 64.01 KB | webflo |
|
#132 | 2447727-132.interdiff.txt | 1.47 KB | webflo |
#132 | 2447727-132.patch | 63.29 KB | webflo |
|
#130 | 2447727-130.patch | 62.64 KB | webflo |
|
#130 | 2447727-130.interdiff.txt | 927 bytes | webflo |
#124 | 2447727-124.patch | 62.51 KB | webflo |
|
#124 | 2447727-124.interdiff.txt | 674 bytes | webflo |
#123 | interdiff-123.txt | 672 bytes | kgoel |
#123 | 2447727-123.patch | 61.85 KB | kgoel |
|
#122 | 2447727-109-122.interdiff.txt | 3.43 KB | webflo |
#122 | 2447727-122.patch | 61.2 KB | webflo |
|
#119 | interdiff-2447727-117-119.txt | 2.68 KB | malcomio |
#119 | 2447727-119.patch | 54.46 KB | malcomio |
|
#117 | add_migrate_support_for-2447727-117.patch | 57.43 KB | vprocessor |
|
#117 | add_migrate_support_for-2447727-117.interdiff.txt | 1.18 KB | vprocessor |
#113 | add_migrate_support_for-2447727-113.patch | 57.53 KB | vprocessor |
|
#112 | interdiff.txt | 2.92 KB | kgoel |
#112 | 2447727-112.patch | 60.67 KB | kgoel |
|
#109 | 2447727-109.patch | 59.88 KB | kgoel |
|
#92 | add_migrate_support_for-2447727-92.patch | 59.86 KB | dimaro |
|
#82 | add_migrate_support_for-2447727-82.patch | 59.79 KB | k4v |
|
#69 | interdiff-2447727-66-69.txt | 4.09 KB | k4v |
#69 | add_migrate_support_for-2447727-69.patch | 60.06 KB | k4v |
|
#66 | interdiff-2447727-62-66.txt | 2.48 KB | k4v |
#66 | add_migrate_support_for-2447727-66.patch | 55.97 KB | k4v |
|
#62 | interdiff-2447727-44-62.txt | 1.6 KB | k4v |
#62 | add_migrate_support_for-2447727-62.patch | 53.5 KB | k4v |
|
#44 | 2447727-44.patch | 53.46 KB | webflo |
|
#43 | 2447727-43.patch | 53.42 KB | webflo |
|
#43 | 2447727-43.interdiff.txt | 6.55 KB | webflo |
#42 | 2447727-42.interdiff.txt | 13.83 KB | webflo |
#42 | 2447727-42.patch | 47.51 KB | webflo |
|
#39 | 2447727-39.patch | 45.36 KB | webflo |
|
#38 | 2447727-38.interdiff.txt | 9.89 KB | webflo |
#38 | 2447727-38.patch | 45.38 KB | webflo |
|
#36 | 2447727-36.patch | 37.08 KB | webflo |
|
#36 | 2447727-36.interdiff.txt | 9.41 KB | webflo |
#34 | 2447727-34.patch | 24.05 KB | webflo |
|
#30 | 2447727-30.patch | 21.62 KB | webflo |
|
#27 | 2447727-27.patch | 19.63 KB | webflo |
|
#24 | 2447727-24.patch | 11.53 KB | webflo |
|
#23 | 2447727-23.patch | 11.65 KB | webflo |
|
#19 | 2447727-19.patch | 14.23 KB | webflo |
|
#18 | 2447727-18.patch | 72.6 KB | webflo |
|
#16 | 2447727-16.patch | 102.22 KB | phenaproxima |
|
#10 | add_migrate_support_for-2447727-10.patch | 628.8 KB | claudiu.cristea |
|
#8 | add_migrate_support_for-2447727-8.patch | 73.16 KB | claudiu.cristea |
|
#6 | add_migrate_support_for-2447727-6.patch | 57.45 KB | claudiu.cristea |
|
#5 | add_migrate_support_for-2447727-5.patch | 31.91 KB | claudiu.cristea |
|
#2 | references.patch | 32.59 KB | claudiu.cristea |
|
Comments
Comment #1
claudiu.cristeaLet's see.
Comment #2
claudiu.cristeaAs the userreference is very similar, I merged the 2 issues. Otherwise there will be a lot of conflicts between the 2 patches because they are touching the same code. Closing #2447729: Migrate D6 user reference fields to D8 entity reference field storage config entities in favour of this.
Here's a first patch. Aspects that still need attention:
referenceable_status
cannot be migrated because we don't have a destination correspondent setting.Comment #3
claudiu.cristeaAdding the parent, meta issue.
Comment #5
claudiu.cristeaComment #6
claudiu.cristeaI made
d6_field_instance
dependent ond6_node
andd6_user
to be able to get the default_value UUID. But I think I'm missing something here related to dependencies. Running onlyMigrateFieldInstanceTest
works fine but the overall test fails.Comment #8
claudiu.cristeaDidn't apply
Comment #10
claudiu.cristeac'mon
Comment #12
benjy CreditAttribution: benjy at CodeDrop commented@claudiu.cristea, just so you know, we don't manually manipulate the dump files anymore, see /core/scripts/migrate-dump-d6.sh
Comment #13
claudiu.cristea@benjy, You maybe reviewed #5.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedNot sure how related it is but I noted that taxonomy references are classified as "node reference" in D8, which sorta surprised me, but in any case, you may want to know about #2506001: Taxonomy term values referenced on nodes do not migrate (D6). Essentially everything comes through except the referenced values (the field is there, and all the taxonmy terms are there).
Comment #15
phenaproximaNot only would this be nice to have, it'd be reasonably easy. All that's needed is a couple of new cckfield plugins.
Comment #16
phenaproximaWIP patch -- adds test fields and values to the Drupal 6 database fixture, and the cckfield plugins to support node and user references.
Comment #17
singularoWhen trying this patch, I got an error:
PHP Fatal error: Undefined class constant 'ENTITY_ID' in /web/app/core/modules/entity_reference/src/Plugin/migrate/cckfield/ReferenceTrait.php on line 26
Changing this to:
'target_id' => static::entityId(),
Allowed things to continue, but not sure that is the correct thing to do.
Comment #18
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedI tried to re-roll the patch.
Comment #19
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #20
phenaproximaThis file is not needed -- drupal6.php is the database fixture and looks like it contains everything required for this patch to work.
Also still needs tests of the migrated reference fields. Other than that, looks great!
Comment #23
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #24
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedER module is dead. Moved everything to Drupal\Core\Field\Plugin\migrate
Comment #27
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #28
phenaproximaNeeds @inheritdoc.
entityId() needs a doc comment.
@inheritdoc
@inheritdoc
This static mapping is not necessary -- ReferenceTrait should implement getFieldType() and always return 'entity_reference'.
Not sure what the point of keeping this is.
The method_exists() check is not necessary because there is already a base implementation in CckFieldPluginBase :)
This @todo can be removed. In this case it's OK to return $global_settings unaltered.
I'm not a huge fan of the way this method is named -- can it maybe be called getStorageSettings() or transformFieldSettings()?
The parameters need to be documented.
Also for consistency, let's lose the $migrate_executable and $destination_property parameters. Ideally, you can even remove the $value parameter and let the plugin read values directly out of $row.
Comment #30
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedAddressed most of the changes from #28 but keep the old
getSettings
method to keep the patch size small. We can remove it in a follow up.Comment #32
phenaproximaA few minor things left, but overall this looks great! Nice work, @webflo.
Nit: id should be ID. Fixable on commit.
Another nit -- why not just return the result of getSettings() in the catch block?
Is this import still needed?
Comment #34
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #35
benjy CreditAttribution: benjy commentedTo be honest i see very little reason to have a trait here, what are we saving, 1 duplicate array definition? Also if we have to share code i'd rather add a MigrateFieldReferenceBase class.
We need to make sure this also fires for D7 field migrations.
Comment #36
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedAdded support for the field instance settings migration today.
Comment #38
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #39
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedd6_field_instance migration depends now on d6_user_role
Comment #41
phenaproximaAs @benjy commented above, it might be better to simply have a base class for reference fields to extend, rather than using a trait.
Needs a doc comment or @inheritdoc.
Typo in the migration ID. :) This should also use // syntax instead of /**/, and explain a little more about why the migration is needed.
Let's turn this into an abstract base class extending CckFieldPluginBase.
Please switch to // syntax and explain why the migration is needed, just so the person who fulfills that todo knows what they're doing :)
I'd rather that the process plugin manager were injected via ContainerFactoryPluginInterface.
This is just a style thing, but you could get away with mocking PluginManagerInterface instead of the Migrate plugin manager.
Also, it's preferable to use Prophecy for tests -- it's a bit more readable than PHPUnit's mocking system.
This has no purpose. ;)
Nit: Should be e.g.
Let's get rid of this stub.
Please remove this.
I believe that d6_filter_format is already executed by migrateContentTypes(), so there's no need to run it again.
Comment #42
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedI did not change the mocking in FieldSettingsTest yet because we rely on the
PluginNotFoundException
which is thrown by the MigratePluginManager.Comment #43
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedI created a new process plugin for the widget type conversion, because the widget conversion is needed in
d6_field.yml
andd6_field_instance_widget_settings.yml
. I think we can remove MigrateCckFieldInterface::processFieldWidget.Comment #44
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #45
mikeryan#2612914: Images transfer but are not linked in content looks related to the general work here on handling entity references - this patch doesn't currently handle files, but given that it's extended to taxonomy terms perhaps we should take that step here as well (as opposed to fixing them in a follow-up)?
Comment #46
mikeryanLooks good to me (forget my previous comment, addressing that in #2604484: Migrate Drupal 7 image and file fields).
Comment #49
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedhttps://www.drupal.org/pift-ci-job/80178 is a random test failure. I filled #2615450: Fix random test failure caused by EntityFile::processStubRow to fix it.
Comment #51
benjy CreditAttribution: benjy commentedWe're already using the process prefix on other functions on this interface, shouldn't we keep inline with that and make these processFieldStorageSettings and processFieldInstanceSettings?
Missing return comment.
Why is this empty?
Comment #52
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedprocess*
andtransform*
are two different things. process is called by the migration builder and transform is called by the migration process plugin.Comment #53
benjy CreditAttribution: benjy at PreviousNext commentedSure, but we're still processing that data and we have process plugins that do the same thing, manipulate data during the migration. That said, process plugins have a method called "transform", happy to hear what others think?
Comment #54
KarenS CreditAttribution: KarenS at Lullabot commentedNeeds a 'use' statement for ReferenceBase.
Needs a 'use' statement for ReferenceBase.
Needs a 'use' statement for ReferenceBase.
Comment #55
KarenS CreditAttribution: KarenS at Lullabot commentedShould be node types not role IDs.
Should be node types not role IDs.
Should be node types not role IDs.
Should be node types not role IDs.
Should be node types not role IDs.
Comment #56
KarenS CreditAttribution: KarenS at Lullabot commentedIt took me a while to realize this, but this code will only work to migrate D6 node and user references. It will not work for D7 node and user references because the field names, widgets, and formatters are named differently. I don't know what might be the best way to extend this code to work for both D6 and D7 style node and user references. It may be that we need totally different classes for each of them because of the differences, i.e. in D7 the original field is 'nodereference' or 'userreference', but in D7 it's 'node_reference' or 'user_reference'.
Comment #57
megastruktur CreditAttribution: megastruktur commentedHi guys! Sorry for a dumb question, but how can I test your patch? How do you test/use it?
I've got a d6 env with lots of noderef entities which I need to migrate to d8. Tried to write my own migration plugin but failed :(
Comment #58
anavarre@megastruktur Easiest way is to make sure you run HEAD (latest dev version of Drupal) and download the patch you want to test against the Drupal codebase, e.g. https://www.drupal.org/files/issues/2447727-44.patch
See https://www.drupal.org/patch/apply - Here's an example of how to do it from within your Drupal codebase on Linux/Mac OSX:
$ wget https://www.drupal.org/files/issues/<patchname>.patch && git apply -v <patchname>.patch
If you see only messages such as
Applied patch /path/to/patched/file cleanly
then things are okay and the patch is ready for you to test, review and report back here.Comment #59
megastruktur CreditAttribution: megastruktur as a volunteer commentedI've tested the 2447727-44 patch on fresh 8.1.x-dev version and it seems to work (everything is imported like a charm)! :)
Can I somehow update the previous migration? I mean, I've already migrated all necessary content and now I need to import noderef fields only. Is it possible?
Or will I have to
1. Remove nodes.
2. Clear migrate cache.
3. Make an import again.
*UPD*: please, tell me, is it essential to use dev version of Drupal? Or 8.0.1 is ok?
**UPD**:
1. It works for 8.0.1 stable release.
2. Cleaning content types, removing all fields and cleaning migrate tables from previously imported content didn't help: content types are imported again WITHOUT noderef fields.
***UPD***:
SOLUTION: rollback the migration and re-run it again. All fields are now in place.
Comment #60
pallavi_sugandhi CreditAttribution: pallavi_sugandhi as a volunteer commentedThanks webflo
I applied the patch 2447727-44.patch on the latest Drupal 8 and then ran the migration process. It successfully imported the node reference fields into the D8 Instance
But still the Taxonomy term reference field is not mapped with the content. When I edit the node it shows -None- in the select list.
Comment #61
chx CreditAttribution: chx commentedComment #62
k4v CreditAttribution: k4v as a volunteer commentedUpdated the comments for #55, the change from #54 is not neccessary I think, the class is in the same package.
Comment #63
k4v CreditAttribution: k4v as a volunteer commentedComment #64
k4v CreditAttribution: k4v as a volunteer commentedComment #66
k4v CreditAttribution: k4v as a volunteer commentedComment #67
k4v CreditAttribution: k4v as a volunteer commentedComment #69
k4v CreditAttribution: k4v as a volunteer commentedThe order is relevant for the test and it was confusing to reuse $expected.
Comment #70
k4v CreditAttribution: k4v as a volunteer commentedComment #71
benjy CreditAttribution: benjy at PreviousNext commentedThis looks great, has anyone tested with a real migration? I'd like to give it a test before we RTBC.
Comment #72
emptyvoid CreditAttribution: emptyvoid as a volunteer commentedI've setup a d6 site with real content, core and all modules are updated to the most recent releases. Running the migration, the fields and data are still ignored. I've applied the patch on a recent release of D8 and files were changed/added.
Perhaps I'm not going through the correct steps?
1. Rebuild manifest
drush migrate-upgrade
--legacy-root='/var/www/hostname/web'
--legacy-db-url='mysql://user@hostname/database'
--configure-only
2. Export to review
drush config-export
-- note none of the node reference fields were detected or written to the exports.
3. Run full migration
drush migrate-upgrade
--legacy-root='/var/www/hostname/web'
--legacy-db-url='mysql://user@hostname/database'
What additional information can I provide to help debug the integration?
Comment #73
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedI think
migrate-upgrade
skips existing migrations. Maybe you had some migrations already in place?Comment #74
miscellainiac CreditAttribution: miscellainiac commentedWe are working to migrate a small Drupal 6 site to Drupal 8. Using drush to rollback the migrations didn't help any, so we threw everything away and started from scratch again. Before migrating, we applied the latest patch linked here. All of our node references came through. For reference we have:
* Event (content) contains a Speaker link that is a node reference to a Person (content)
* Publication (content) contains an Authors link that is a node reference to a Person (content)
* Research (content) contains links to Member, Related Event, and Related Publications that are node references to a Person, Event, and Publication
Comment #75
infopicard CreditAttribution: infopicard commentedHello,
I have the same requirement to migration a lot of CKK node references from D6->D8
but I'm not familar with patches ;-)
is it realistic that this migration option becomes release in one of the next drupal 8 releases (core or plugin)?
Comment #76
infopicard CreditAttribution: infopicard commentedHi Benji,
what could I do to assist you. I have a big D6 website with a lot of node references in use.
My first migration was particular successfull excluding node reference field for example. (location fields also missed and some other issues, but this will be another battlefield)
Could I do any tests? Which environment should i use? I tested it with current D6 Version an D8.0.3
perhaps if I could get the code for this 8.0.3 (if even possible) I could check it - thx.
Comment #77
scottishlass CreditAttribution: scottishlass commentedApplying patch #69 failed for me on latest 8.0.x-dev. Does it need a re-roll?
Checking patch core/modules/field/src/Plugin/migrate/process/d6/FieldInstanceSettings.php...
error: while searching for:
$settings['prefix'] = $field_settings['prefix'];
$settings['suffix'] = $field_settings['suffix'];
...
Comment #78
scottishlass CreditAttribution: scottishlass commentedApologies didn't mean to change status, setting back to Needs review
Comment #79
miscellainiac CreditAttribution: miscellainiac commentedApplying the patch to 8.0.4-dev fails; however if I pin Drupal to 8.0.3 then the patch is applied successfully.
Comment #80
infopicard CreditAttribution: infopicard as a volunteer commentedOk Applying the patch to 8.0.3 was successfull
but update-migrate don't recognize my node reference fields
message: Source ID node,teaser,contenttype,field_1: Failed to lookup field type array ( 0 => 'nodereference', 1 => 'default', ) in the static map.
did I something wrong oder what info do you need to figure out the problem?
Edit: forget it. the node reference migrated successfully !!! THANKS!!!
I migrate aprox 5000 references in a couple of different fields. Perfect!
Comment #81
benjy CreditAttribution: benjy at PreviousNext commented#74 and #80 both report this working as expected, looks like the patch needs a re-roll then RTBC from me.
Comment #82
k4v CreditAttribution: k4v as a volunteer commentedreroll
Comment #83
k4v CreditAttribution: k4v as a volunteer commentedComment #85
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedThe latest patch apply correctly.
Remove tag Needs reroll.
Comment #87
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commentedAdding the tag drupalconasia2016 as we are working on this issue during the sprint.
Comment #92
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedRerolled #82.
Comment #93
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedApologies, update to run the test.
Comment #95
quietone CreditAttribution: quietone as a volunteer commentedComment #96
el1_1el CreditAttribution: el1_1el commentedI've now used both 44 and 92 over the past 2 months. Both seem to be working fine for multiple nr and ur fields
Comment #97
benjy CreditAttribution: benjy at PreviousNext commentedWe just need the two test fails investigating and then we can get this ready.
Comment #98
el1_1el CreditAttribution: el1_1el commentedI got both the errors reported in 92 on git apply w core 8.0.4 but the patch applied cleanly after upgrading to 8.0.5
Comment #99
infopicard CreditAttribution: infopicard as a volunteer commentedI tested it successfully against 8.0.5 and a bigger D6 site with a lot of node references. It works fine for me!
Comment #100
quietone CreditAttribution: quietone as a volunteer commentedUser reference is enabled in the dump but Node reference is not. Surely, they should both be enabled.
Comment #101
cosmicdreams CreditAttribution: cosmicdreams commented@benjy: At the time of writing this comment, the testbot has retested this patch last on March 31st. That test run shows that all tests passed. I don't know why that isn't reflected in the main issue here, but if you click into the data on how the tests were last run you'd see that.
Comment #102
benjy CreditAttribution: benjy at PreviousNext commented@cosmicdreams, looks pretty clear to me that two tests failed here: https://www.drupal.org/pift-ci-job/197704 Maybe you're not talking about the patch in #92?
I've re-tested the patch in #92 just to make sure.
Also, at this point we should probably be re-rolling this against 8.1
Comment #104
cr0ss CreditAttribution: cr0ss as a volunteer and commentedHello,
I have D6 site with Blog node type referenced to Series node and D8.0.5 with applied #92 patch.
Steps I took:
1. Applied patch #92
2. drush cr all
3. drush migrate-upgrade --legacy-db-url=mysql://user:pass@localhost/d6 --configure-only
4. I got field_series: field_series in migrate.migration.d6_node__blog.yml
5. drush migrate-import d6_node__series
6. drush migrate-import d6_node__blog
None of references were created. What I do wrong?
Comment #105
malcomio CreditAttribution: malcomio commentedThe patch in #92 applied cleanly against the current 8.0-x branch, on a fresh installation of the standard profile, and node and taxonomy term references were successfully migrated, after following these steps:
However, there were several problems with the migration - most notably that image fields were not migrated (which they are without the patch applied)
Comment #107
malcomio CreditAttribution: malcomio commentedLooks like the patch needs significant changes to work with 8.1.x - was hoping to be able to re-roll it, but I think it's not a small job :(
Comment #108
cr0ss CreditAttribution: cr0ss as a volunteer and commentedHey @malcomio, would you mind posting how the migrate.migration.d6_node__[node_type].yml that has node references field looks for you?
I'm still having a problem with migrating node_references, just gave it a try on a latest 8.0.x-dev version.
Comment #109
kgoel CreditAttribution: kgoel commentedRerolled.
Looking into fails.
Comment #111
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #112
kgoel CreditAttribution: kgoel commentedFails in #109 were mainly because of the change introduced in https://www.drupal.org/node/2676222
Comment #113
vprocessor CreditAttribution: vprocessor at Skilld commentedHi guys, patch is ready
Comment #116
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #117
vprocessor CreditAttribution: vprocessor at Skilld commentedremoved duplicated interfaces
Comment #119
malcomio CreditAttribution: malcomio at Capgemini commentedApplying the patch against 8.1.x, I get the following error when trying to run drush migrate-upgrade --configure-only
Seems to be happening because the Migration class is in the wrong namespace. I've re-rolled the patch, although I still need to test it more.
Comment #121
malcomio CreditAttribution: malcomio at Capgemini commentedWith the patch applied, my migrations are falling over with an error that suggests that something is going wrong in the inheritance somewhere, and the D7 Vocabulary migration plugin is being used instead of the D6 one for some reason.
Comment #122
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedTried to fix the failing test from #112. Patch is based on #112
Comment #123
kgoel CreditAttribution: kgoel commentedComment #124
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #128
malcomio CreditAttribution: malcomio at Capgemini commentedGood news is the patch seems to work for me on a clean install of 8.1.x.
Now for the testbot failures...
Comment #129
infopicard CreditAttribution: infopicard as a volunteer commentedI tested it successful too
Comment #130
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedField and Field Instance Migrations depends on d6_user_role and filter format, because user_reference fields user role related settings.
Comment #132
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedE-Mail Widget got size attribute in #2578741: Add setting for size to email widget
Comment #133
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #136
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedMaybe we should sort the array or make the assertions smaller.
Comment #137
Chithra K CreditAttribution: Chithra K as a volunteer and at Zyxware Technologies commentedHi,
I tested a d6 migration with node and user references after applying the patch 2447727-135. It is working.
Comment #138
vprocessor CreditAttribution: vprocessor at Skilld commentedComment #139
quietone CreditAttribution: quietone as a volunteer commentedA bit of a review. I focused on code sniffer because I've just set it up in PhpStorm.
Unused.
Where are get and set defined? PhpStorm can't find them.
Unused.
Expected hasn't been used yet, so this is not strictly necessary.
For consistency with the rest of the file, use short array syntax, [].
Unused.
Not used.
Comment #140
miscellainiac CreditAttribution: miscellainiac commentedHello all,
We're trying to migrate once again from a D6 site to a D8 site, and surprised to find that this patch hasn't been rolled into a core release yet...
Regardless, applying the patch is fine, but when we try to migrate the migration just dies with the following:
This is not helpful at all. We don't know what field is failing to migrate, if we knew that, we could remove the problematic fields and do some post-migration stuff to move that data over.
Comment #141
Anonymous (not verified) CreditAttribution: Anonymous commented@miscellainiac were you getting this error before applying the patch? I suspect it is an unrelated issue, but it could also be related to the field settings if migrate isn't handling it correctly. If you are able to find out what field is failing can you post it here, or open a new issue if it is unrelated?
Comment #142
miscellainiac CreditAttribution: miscellainiac commented@ryan-weal I should have put that in the post; yes without the patch we don't see this error. What's more, we've tried with an older version of Drupal Core (8.0.3) and the matching patch (also in this thread) and a compatible version of the migrate_upgrade module. Same error.
I was hoping that someone might know where we can inject some code to dump out what field the module is trying to migrate. For example, I added some print_r statements to the code (in the patch) that processes a node reference so I can see what content type it chokes on. So we removed that content type, my print_r statement is never reached, but the migration still dies with the same error.
At this point I have to binary-search slash-n-burn our content types and fields to figure out something that should just be dumped out to the console during the migration anyways...
Comment #143
miscellainiac CreditAttribution: miscellainiac commentedFor some more information, I was able to narrow down the content type with the problem. With have a content type of Person - that content type has a User Reference field in it. If I remove that field from the content type, everything seems to migrate just fine. However, we performed a migration back with Drupal Core 8.0.3 and the patch here (different site though) and the User Reference field came over as expected...
Comment #144
Anonymous (not verified) CreditAttribution: Anonymous commentedSounds like you found a bug. It would probably be worth asking for help in #drupal-migrate on IRC. If you are at the sprint in New Orleans tomorrow I'm sure there will be a migrate table.
Comment #145
miscellainiac CreditAttribution: miscellainiac commentedI didn't have much luck in the IRC channel last time we ran into migration issues, so I'm not inclined to spend any time there now. Could someone let me know where I can report the bug? It's very possible that the data is just bad in our case, but I was hoping for a more graceful failure scenario and some debug output so a developer can troubleshoot...
Comment #146
quietone CreditAttribution: quietone as a volunteer commented@miscellainiac, This may help with reporting the issue, How to create a good issue.
Comment #147
tlyngej CreditAttribution: tlyngej at Annertech commentedRe-roll the patch to the current 8.1.x code base.
Comment #148
quietone CreditAttribution: quietone as a volunteer commentedSetting to NR to test the patch in #147.
Comment #149
claudiu.cristeaThe @file statement should not be added anymore to classes with namespace definition.
Comment #151
claudiu.cristeaPatch doesn't apply anymore.
Comment #152
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedRerolled #136.
Hope it works!
Comment #154
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedRe-roll against after #2631736: Cckfield Plugins must distinguish core versions got in.
Comment #155
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #158
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedcckPluginManager needs the core version
Comment #160
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedUploaded the wrong patch yesterday.
Comment #162
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedUpdated the field_config and field_storage_config entity counts in MigrateUpgrade6Test::getEntityCounts
Comment #163
chipway CreditAttribution: chipway at Chipway commentedSorry @webflo,
I have not the right D6/D7 website copies to test it here in Milano and it took me a long time to read it, but Patch 2447727-162.patch applied successfully.
As this patch seems to solve the issue, I would hope it to be committed asap to reduce review complexity and time.
I will come back in a few weeks to try to go further in reviewing your great work.
Thks
Comment #164
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedRerolled after #2687003: Remove references to nonexistent functionality from migrate.api.php has been committed.
Comment #165
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedComment #167
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedAdded a wrong file to the patch.
Comment #169
mikeryanRetesting, failure looks unconnected.
Comment #171
quietone CreditAttribution: quietone as a volunteer commentedPassing test now, back to NR.
Comment #172
phenaproximaThis is an epic patch. Thanks for your hard work on this, @webflo et. al!
I'm pretty sure that Node Reference existed for D7 as well.
Needs a description.
Should this be a constant?
Ideally this should be injected.
Not sure this doc comment is accurate. According to create(), this is the process plugin manager.
IMHO this should be part of the plugin definition, rather than a method. Not a big deal though.
Isn't this already implemented in CckFieldPluginBase?
taxonomy_term_reference fields didn't exist in D6, to my knowledge.
This hasn't got any handler settings? (Valid vocabularies that can be referenced, etc.)
Wasn't User Reference also available for D7?
Same as in NodeReference -- can/should this be a constant? Needs a description either way.
This is very similar to NodeReference::migrateNodeTypes(). Can it be made into a generic method of ReferenceBase?
Why is the plugin ID field_field_settings but the class name is FieldInstanceSettings?
There's an unnecessary empty line here.
The plugin ID doesn't have a d6_ prefix, so why is the core version hard-coded to 6?
Why is the core version hard-coded to 6 if the plugin ID doesn't have the d6_ prefix?
Let's use assertSame() rather than the deprecated assertIdentical().
s/node/user
Same here.
assertSame() instead of assertIdentical().
Can we use Prophecy here?
There's an errant extra blank line after the doc comment. :)
Let's add a default case, for completeness' sake.
Not a big fan of the copy-paste with the D7 version of this class. Should this be moved into a base class?
Missing default case.
Comment #173
benstjohn CreditAttribution: benstjohn commentedPatching is failing using fresh drupal 8.1.8. This is rejected file:
It looks to my like the original file: MigrateUpgrade6Test.php has its field_config value set to 63, not 62, tweaking that in the patch fixes it although I'm not really sure what the implications of that are? Can someone weigh in?
Comment #174
phenaproximaSounds like this needs a reroll against 8.2.x.
Comment #176
hussainwebRerolled. Once tests pass, this should be back to needs work for #172.
Comment #177
hussainwebNeeds work as per #172.
Comment #178
hussainweb#172,
1, 10: I'd suggest to focus just on D6 here.
2, 4, 5: Done
3, 11: added descriptions
7: No, this is not defined in the base class. It has to be implemented by concrete classes.
8: This is copied from the already existing TaxonomyTermReference plugin. But you're right. I am thinking we should deal with that in a follow-up.
12: Done, but I think we can improve the naming of the method.
13: It's not changed here. I think the reason is that instances were just called fields in D6?
14: Fixed
15: Actually, this plugin is only for D6. There is a "d7_field_settings" for D7.
16: See 15.
17, 20: There are lots of other assertIdentical in the test. I think we should do this in a follow-up. I recall you raised this in another issue as well. We could combine this for all of them.
18, 19: Fixed
22: Fixed
23, 25: What should the default case do?
24: The only common base class here is CckFieldPluginBase and it doesn't make sense there. Could we inherit the D7 version from D6 like we do elsewhere?
This patch adds a lot of features to ReferenceBase for field migrations. I think some of these would make sense for D7 as well. Follow-up?
I also realize I skipped a couple of points I was not very clear on. There were lot of changes already, so we could look at them again after tests and review.
Comment #179
phenaproximaI'm not sure I agree that it should be in a follow-up, because we would be committing something that is flat-out incorrect. At the very least, let's remove 6 from the plugin's core array.
Yeah, it's weird. I'm not TOO bent out of shape about it, but ideally I'd like to fix it ASAP.
Okay, fair enough. I agree and will stop raising this issue until I open a follow-up :)
Explicitly return NULL or FALSE so that migrations can skip on empty.
If the D6 and D7 versions are very similar, I'm open to that.
Comment #180
phenaproximas/id/ID
The migration ID is not hard-coded.
s/id/ID
$source_ids should be type hinted.
Shouldn't say d6_user_role.
s/id/ID
Needs a blank line between the description and @var.
Comment #182
hussainwebRerolling first
Comment #183
phenaproximaThe reroll seems legit, but I'd like the stuff in #180 addressed before RTBC.
Comment #184
keithm CreditAttribution: keithm commentedEdited #182 to address #180 open items.
Comment #185
keithm CreditAttribution: keithm commentedComment #186
phenaproximaI'm really sorry to do this.
We're assuming here that $source_settings['referenceable_types'] exists -- and it should, but you never know. The plugin should play defense here and simply return $settings as-is if 'referenceable_types' isn't set in $source_settings.
It's not just role IDs were looking up here. I think this method should be renamed to more accurately reflect what it does -- perhaps, lookupMigratedBundles() or similar, and the doc comment must be fixed to match.
Taxonomy term reference fields did not exist in D6. I'd rather we just said {7} here, because this is the sort of thing that will fly under the radar and potentially cause weird bugs far down the line. Let's nip them in the bud.
Let's be defensive here as well, in case some weirdo source database doesn't give us $source_settings['referenceable_types'].
What is the nature of this change? What will this accomplish?
I don't like the fact that ['core' => 6] is hardcoded here. Can this instead be a configuration value for this dispatch plugin?
I'd also prefer if the value for type was passed in as $value, rather than pulled directly out of the row.
These should not be pulled directly out of $row -- they should be passed in as $value.
Again, I'd rather the core version were passed in with plugin configuration rather than hard-coded.
assertIdentical() is deprecated -- PHPUnit uses assertSame(). I know the tests are still using the old method, but for the future's sake let's use assertSame() now and change all the old assertions in a follow-up.
Why array()? We should use consistent PHP 5.4+ syntax. Please change all of these to [].
Let's do a check to ensure that $field_settings['title'] is actually set.
Comment #187
iMiksuI'll work on some of the items now.
Comment #188
iMiksuFinished working on comment #186 items 1, 3, 4, 9, 10 and 11.
Comment #189
quietone CreditAttribution: quietone as a volunteer commentedLets test patch #188
Comment #190
phenaproximaIt's not obvious, but these are actually constants defined by core, so I'd prefer to use the actual constants rather than hard-code the integer equivalents. 0 = DRUPAL_DISABLED, 1 = DRUPAL_OPTIONAL, and 2 = DRUPAL_REQUIRED.
Comment #191
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedApply changes mentioned above on #190
Comment #193
phenaproximaI have read, re-read, reviewed, and even tried to refactor parts of this patch. This has led me to the conclusion that, for several reasons, we cannot continue with this patch as-is. I'm closing this issue so that we can pursue the noble goal of migrating Drupal 6 and 7 reference fields in a more sane, consistent, and compartmentalized way.
I feel bad about effectively snubbing the hard and good work of so many people, but this patch has several problems that will make it hell to finish, much less get committed. It is littered with out-of-scope changes that, as far as I can tell, don't quite relate to the task at hand -- changes which haven't necessarily been reflected in changes to the test coverage. These changes confuse the issue and make it harder to review (and harder to find potential bugs). I think the reach of this patch exceeds its grasp.
I intend to open follow-up issues which divide this big task (reference field support) into smaller parts. First we can support Drupal 6 node references, then user references, then ditto for Drupal 7, and so forth. I think this will make it a great deal easier (and faster) to get this completely done for 8.3.x.
Comment #194
phenaproximaArise, Lazarus!
After discussing with @benjy on IRC, I think we can reopen this issue in the name of not disrupting and discarding all the work that has been done here, with the proviso that we drastically reduce the scope of the patch.
I think that the latest iteration of the patch contains a particularly excellent idea, which is the ReferenceBase base class upon which other reference-type field plugins can be built. They are generally very similar to each other, so I think it makes sense for ReferenceBase to be turned into its own, manageably small patch (in this issue) and committed first, followed closely by subclass implementations created in follow-up issues.
Let's do this.
P.S. To avoid reroll hell, I am postponing this issue on #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins.
Comment #195
phenaproximaApparently when I say "postponed", I mean "needs work".
/me is an idiot.
Comment #196
phenaproximaComment #197
quietone CreditAttribution: quietone as a volunteer commented#2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins is in so this is no longer blocked.
Comment #198
heddnAnd #2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins got reverted, so back to postponed.
Comment #199
catchTagging and bumping status since this blocks several migrations like node reference and user reference.
Comment #200
phenaproximaComment #202
quietone CreditAttribution: quietone as a volunteer commented#2683435: CCK does not exist in Drupal 7, rename Migrate's cckfield plugins to field plugins has been committed to 8.4.x.
Changing this to 8.4.x, setting to NW and tagging for reroll.
Comment #203
quietone CreditAttribution: quietone as a volunteer commentedReally set to NW
Comment #204
jofitz CreditAttribution: jofitz at ComputerMinds commentedThat was a tough re-roll!
Comment #206
heddnTagging to sprint on in Baltimore.
Comment #207
heddnWith most of the reference from d6 mostly done, the priority here can be dropped. I've confirmed this with @phenaproxima in IRC as well.
Comment #208
mikeryanComment #209
jofitz CreditAttribution: jofitz at ComputerMinds commentedPatch no longer applied so re-rolled.
Comment #211
jofitz CreditAttribution: jofitz at ComputerMinds commentedTransfer functions out of the deprecated class CckFieldPluginBase.
Comment #213
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrecting mistakes in the D6 fixture.
Comment #215
kubrt CreditAttribution: kubrt commentedHi,
great work on this so far. Is there any way to use this patch with the current 8.3.3 core to migrate nodereferences from D6 to D8 ?
Any pointers greatly appreciated.
Comment #216
phenaproximaAdding #2886801: Migrate D6 user reference values to D8 as a related issue, since all reference field issues seem to be coalescing around this one.
Comment #217
joelpittetAn attempt at a re-roll. Probably messed up in
FieldSettings::transform
Comment #219
joelpittetThe errors are related to
Undefined variable: source_field_type
mostly, but still thinkFieldSettings::transform
I did wrong, as it's unclear what it's doing and how the BC works.Comment #221
joelpittetComment #222
quietone CreditAttribution: quietone as a volunteer commentedHad a go at the reroll. I started with the patch in #213.
Comment #224
jofitz CreditAttribution: jofitz at ComputerMinds commentedHopefully this will correct more test failures than it creates...
Comment #226
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #228
jofitz CreditAttribution: jofitz at ComputerMinds commentedPatch in #226 no longer applies. Re-rolled (prior to addressing test failures).
Comment #230
jofitz CreditAttribution: jofitz at ComputerMinds commentedHaving spent hours pouring over this code, I've made a few discoveries/improvements:
Clearly that 4th change is a flimsy band-aid until a robust solution is found, but I just wanted to get this moving. I recommend we try and solve that in a follow-up so that this code can get committed. I also wonder if this would be an issue with other similar fields, e.g. term/user reference fields?
Comment #232
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrected D6 field_storage_config entity count.
Comment #233
quietone CreditAttribution: quietone as a volunteer commented@Jo Fitzgerald, thanks for continuing to work on this.
I looked at this a bit too late in the day, my tired brain isn't up to a review. I did fix a few missing commas, fixed in the attached patch.
Comment #235
maxocub CreditAttribution: maxocub as a volunteer commentedNeeds reroll.
Comment #236
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedThanks everyone for working on the issue. Here is the rerolled patch :)
Comment #238
jofitz CreditAttribution: jofitz at ComputerMinds commentedReroll of patch in #233.
Comment #240
jofitz CreditAttribution: jofitz at ComputerMinds commentedHandle the mapping between
nodereference_autocomplete
andentity_reference_autocomplete_tags
as removed from d6_field_instance_widget_settings.yml.Comment #241
yogeshmpawarComment #242
yogeshmpawarRe-rolled the #240 patch against 8.6.x
Comment #244
yogeshmpawarUpdated patch will solve the test fails, also added an interdiff.
Comment #246
yogeshmpawarOne more try to resolve test fail issue with an interdiff.
Comment #247
phenaproximaI'm sorry to say that I think this patch needs significant refactoring.
The thing is, the current patch makes a lot of changes to field plugins that have nothing to do with reference fields. While these changes are a very welcome DX improvement, they are way out of scope here and need to be dealt with in follow-up issues. As it is, they creates a lot of patch noise that makes review and commit close to impossible in a timely manner. Let's revert as much of this patch as possible and change only the things that affect reference fields.
Once that's done, we may need to clear this with a framework manager because adding methods to MigrateFieldInterface may constitute a BC break. Which, if we can even get away with it, will need a change record as well. But that's putting the cart before the horse; first, let's clean things up so that we're making the fixes we actually need, rather than all the ones we ant.
Comment #249
edysmpI have a issue with drupal 6 to drupal 8 migration with nodereference fields, they are not migrated with their target_bundles. This issue seems fixing this too, do it?
Comment #250
quietone CreditAttribution: quietone as a volunteer commentedJust a reroll for 8.7.x
Comment #252
quietone CreditAttribution: quietone as a volunteer commentedTrying to address #247 and remove all changes that are not related to reference fields. The removed changes need to go to a new issue.
Comment #254
quietone CreditAttribution: quietone as a volunteer commentedAdjust the entity counts for the addition of 4 fields.
Comment #255
heddnReally great work here on a long-standing issue. I think we're pretty close.
Could we also add test coverage for target_bundles to address concerns in #249?
I wonder if these should move into a d6 namespace? Since they are obviously only for D6.
Comment #256
quietone CreditAttribution: quietone as a volunteer commentedNeeds a reroll
Comment #257
quietone CreditAttribution: quietone as a volunteer commented255.1 Adding a test will only show it doesn't work. That is an existing bug and, in my opinion, should be in a followup. where the removed code here can be used to fix that. The target_bundles are migrated in a new method transformFieldInstanceSettings which is being removed here according to #247.
255.2 This patch moves the plugins to a d6 subdirectory.
And removes the new method lookupMigrations and transformFieldInstanceSettings which does not need to be done to make a base class. Add doc comments and other cleanup.
If this passes, the next piece is to look at any other reference migrations that can use the new base class, such as taxonomytermreference.
Comment #258
quietone CreditAttribution: quietone as a volunteer commentedI've skimmed through the comments and found that phenaproxima said in #172 that user and node reference exist in Drupal 7, which is different that what heddn says in #255.
Can someone clarify that?
Comment #259
quietone CreditAttribution: quietone as a volunteer commentedmikelutz answered on slack that it should be d6 and d7 so moving back.
Comment #260
quietone CreditAttribution: quietone as a volunteer commentedRe #249 and target bundles. My comment about that in #257 is wrong. The work to migrate target_bundles and the tests are, or rather were, in this issue. They are now moved to #3033522: Add methods to customize field to MigrateFieldInterface so that this patch is only making the ReferenceBase class.
Comment #261
mikelutzI was talking with Damien McKenna and John Ouellet in slack, and figured out the confusion with the drupal versions. Currently in code, we seem to support d6 nodereferece and userreference fields, but we don't actually have field plugins for D7 reference fields (node and user). D7 term reference fields were part of core, but node and user references were in the contrib references module, which we don't but should support.
I don't know, but I suspect they might need new field plugins separate from the d6 ones, so we might want the plugins in a d6 folder and a followup to write new ones for D7. I need to do a bit more research.
Comment #262
heddnSo, for a base field maybe we want to provide a getter for
$nodeTypeMigration
and$userRoleMigration
that is abstract? Then leave these in the global namespace and make a d6 version that implements those methods.Comment #263
mradcliffeI'm debating whether or not it's worthwhile to also include the test fixture and test I wrote in #3008202: Migrating more than 1 entity reference field instance fails to this issue and closing it. That test confirms that an entityreference entity reference used on multiple bundles of an entity do not get migrated to entity_reference fields correctly and field definitions are missing.
It would probably be confusing to users if this issue were completed, but they would still end up with a broken web site if they re-used entityreference fields in Drupal 7 when saving an entity reference field on one of those nodes with broken field definitions.
On the other hand the patch is rather large already.
Comment #266
Wim LeersThis blocks #2814953: Migrate Drupal 7 node/user reference fields.
I increased the priority of that issue to
based on:Matching priority.
Comment #267
mradcliffeThis an attempt at a re-roll of the patch in #259 and incremented the counts in Upgrade6Test from what they are in 8.9.x by 4, which is what the patch in #259 is doing.
This is still Needs work based on #261 + #262, and I think that means reworking the changes in #257.
Comment #268
mradcliffeI'm going to work on re-rolling and fixing the migration count changes that I introduced in #267.
Comment #269
mradcliffeI went back and counted up the field config manually. I think I thought there were 4 more added to core, but that wasn't the case. I changed the assertion in FieldDiscoveryTest to assert the contents of the array rather than the explicit order.
The interdiff is a little strange because the hunks changed.
I have some more time so I'm going to try to move NodeReference and UserReference back into a d6 folder like @quietone did in #257 and @mikelutz suggests in #261.
Comment #270
mradcliffeThis patch moves NodeReference and UserReference back underneath a d6 directory and updates the namespaces in those files.
It also addresses a test failure in #269.
This doesn't attempt to implement any getters mentioned by @heddn in #262, which I am confused about what the purpose of splitting the protected property is for.
Edit: I'm finished working on this for the day.
Comment #272
mradcliffeI think I understand what @heddn was suggesting. Here's a new patch that creates an abstract protected method on ReferenceBase that takes a MigrationInterface as a parameter. This should allow some amount of flexibility for contrib, custom, or core plugins that implement it.
I renamed the protected properties on d6/UserReference and d6/NodeReference to the ones that @heddn suggested. I still don't understand the comment about the "global namespace", but hopefully this meets expectations. This also removes the referenceTypeMigration property from ReferenceBase.
I marked the plugins as @internal because core plugins are internal.
Previous patches also made this change:
This is giving a warning in phpstorm because the plugin class doesn't implement a __construct method that takes 5 arguments. It's using
\Drupal\Component\Plugin\PluginBase::__construct(array $configuration, $plugin_id, $plugin_definition);
I'm not sure about the intent of the change here? Are $migration_plugin_manager and $migrate_process_plugin_manager necessary? It does not affect the test run of the unit test. I've reverted this change in this patch.
Comment #273
heddnVery nice work! You dissected my fairly obtuse comments quite nicely. A few small comments.
:thumbsup:
Would it make sense to mark these as final? It would, but not sure what that means for BC concerns. Ideally we would. But :shrug: I guess we can't at this point.
Naming is hard. What if we called this
getEntityTypeMigrationId
? Would that make more sense to anyone else?Comment #274
Neslee Canil PintoComment #275
heddnI just noticed this, why do we need to provide an argument to this method? We could be fine without it, right?
Comment #276
Neslee Canil PintoComment #277
heddnIf this returns green. Then I like it!
Comment #278
quietone CreditAttribution: quietone as a volunteer commentedReally glad to see the progress here. I found a few nits.
Why are these migrations needed for taxonomy translation? I applied the patch and ran the test without them and it passed.
Unused parameter, can be removed.
Not used, can be removed.
I don't think a theme needs to be enabled. This can be removed.
Including the filter_format and user_role migration in the field migrations is unexpected. I'd rather see this removed and those migrations only added to the tests that need it.
Comment #279
Neslee Canil PintoComment #280
mradcliffe@heddn, I added the parameter because the method seemed sort of basic without one. I thought it would make sense for a contrib, custom, or other core plugin might need to access the migration entity to calculate the correct entity id. Otherwise the method seems fairly limiting.
It could be an API addition later, but harder to do later.
Comment #281
quietone CreditAttribution: quietone as a volunteer commented@Neslee Canil Pinto, thanks for the changes. It would be easier for me to review if you added a comment stating what you changed.
And one more nit.
This changed can be removed.
Comment #282
Neslee Canil PintoHi @quietone,
Removed d6_filter_format and d6_user_role.
Removed the @param.
Removed the unused use statement.
Removed schema_version key.
Removed d6_filter_format and d6_user_role.
Comment #284
Neslee Canil PintoWe need schema_version key, #279 Works Fine
Comment #285
mradcliffeI think @quietone wanted to schema_version to be set, to -1, not removed.
Flipping back to Needs work because the latest patch is broken.
Comment #286
Neslee Canil PintoComment #287
quietone CreditAttribution: quietone as a volunteer commentedThanks for the fixes, sorry about the confusion on the 'schema' line in the Drupal6 test fixture.
I have applied the patch and checked the changes since #278 and they all are fixed. This can go back to RTBC.
Comment #289
quietone CreditAttribution: quietone as a volunteer commentedComment #290
quietone CreditAttribution: quietone as a volunteer commentedA simple reroll
Comment #291
mradcliffeAssertions look the same between #286 and #291. Going to flip to RTBC based on the review in #287.
Comment #292
quietone CreditAttribution: quietone as a volunteer commentedThanks for restoring the RTBC.
Onoe small change to make.
I think this should use assertCount()
Comment #293
quietone CreditAttribution: quietone as a volunteer commentedFix for #292.
Comment #295
quietone CreditAttribution: quietone as a volunteer commentedUnrelated failure in Drupal\Tests\media_library\FunctionalJavascript\EntityReferenceWidgetTest
Comment #296
mradcliffeHmm, there haven't been any changes that would seem to effect this, but it's still failing randomly (?). I don't know why this patch would cause that to fail, and HEAD isn't broken on 9.1.x.
Comment #297
steinmb CreditAttribution: steinmb as a volunteer commentedLooks like RTBC to me.
assertCount()
seems like the right change.Comment #298
quietone CreditAttribution: quietone as a volunteer commentedClosed #3048373: Streamline entityreference field migration processes as a duplicate.
Comment #299
Nuuou CreditAttribution: Nuuou commentedMy team has been working on some Drupal 7 to Drupal 8 (or 9) migrations, and this is something we obviously encountered.
I'm fairly confused by the state of this though, as the initial request seemed to be about both Drupal 6 and Drupal 7. However, unless I'm misunderstanding this, it appears the only included migration path in this update is for Drupal 6.
Applying this patch does not seem to migrate Drupal 7 References module (not entityreference module) to Drupal 8/9. Is that correct, or am I encountering something else possibly?
Comment #300
mradcliffeYes, that's correct, @Nuuou. This issue is about adding a base class that can then be used by the follow-up issues for migrations for D7 references, D7 entity reference, etc...
Comment #301
quietone CreditAttribution: quietone as a volunteer commentedThere is a patch at 2814953-#33 for Drupal7. I have not tested it.
Comment #302
mradcliffe#293 is failing to apply to 9.1.x, but status didn't update. Flipping the status to Needs work and and adding the Needs reroll tag.
I'll try re-rolling it again.
Comment #303
mradcliffeThe patch applied cleanly when I used a 3-way merge (
git apply -3
). I noticed that the current patch usedassertIdentical
test assertion method that was deprecated in Drupal 9. The only difference in the patches after the re-roll was this.Uploading the patch for testing. Hopefully it still passes.
I also updated the issue summary removing the outdated part of remaining tasks.
I realize at 300+ comments, we're now at the second page, which makes it significantly harder for a core committer to review. What can we do to make it easier review the next time the issue is RTBC?
Comment #304
quietone CreditAttribution: quietone as a volunteer commented@mradcliffe, thanks for the reroll. It look good to me.
+1 for RTBC
Comment #305
simeThe API changes in the IS are out of date, and seem to be related to #272 patch. Have updated.
Code reads logically. I ran the affected tests files successfully. I tweaked the fixture data and caused the relevant tests to fail as expected.
Since @quietone has voted RTBC, and this was RTBC before the patch got stale, and the interdiff in #303 adds no logic, I'm RTBC.
Comment #306
quietone CreditAttribution: quietone as a volunteer commented@sime, thank you very much! Your review and testing will help the next reviewer. You see, in the meantime, jibran pointed out that deleting NodeReference was a BC break, so more work to do.
Which I have attempted to to do now. The IS is updated to include the deprecation, a change record has been added, deprecated \Drupal\migrate_drupal\Plugin\migrate\field\NodeReference and a test has been added.
Comment #308
quietone CreditAttribution: quietone as a volunteer commentedThere were some coding standards to fix. I've not figured out why the new test is failing. It passes with phpunit but then I found that run-tests.sh isn't working on any of my dev environments so that is no help to me right now.
Comment #309
quietone CreditAttribution: quietone as a volunteer commentedI am sure I checked the namespace several times.
Comment #310
mradcliffe+1 on adding the backwards compatibility class.
Is this supposed to be ImageField or \Drupal\migrate_drupal\Plugin\migrate\field\d6\NodeReference?
Comment #311
quietone CreditAttribution: quietone as a volunteer commented@mradcliffe, thanks. It's a copy/paste error. This should fix it.
Comment #312
Kristen PolThanks for the update.
1) I reviewed from #305 and onward as there are a LOT of comments. :)
2) Issue was RTBC in #305 but patch was updated in #306 to #309 to address a backwards compatibility issue for
NodeReference
.3) The only interdiffs I reviewed were the ones from #306, #308, #309, and #311.
a) #306 adds a deprecation for
NodeReference
and a test.b) #308 addresses code standard issues from #306.
c) #309 addresses name space issue from #308.
d) #311 addresses #310 for a wrong class.
4) Patch applies cleanly and tests pass.
5) Read the change record and made some minor formatting changes.
6) Title and issue summary are clear though there is a question on whether documentation will need to be updated based on this so there may need to be a follow-up issue.
7) Note that the release notes snippet does need to be filled in.
Back to RTBC based on the above.
Comment #313
quietone CreditAttribution: quietone as a volunteer commentedAnd now a reroll. The only change was to UserReference.php
There is no documentation to update, at least none that I could find.
Comment #314
Kristen PolMoving back to needs work as it looks like a bit of the patch from #311 was not preserved.
Diff:
Previous patch:
Comment #315
quietone CreditAttribution: quietone as a volunteer commentedArg! I got interrupted and didn't check my work. Rerolled again against #311.
Comment #316
Kristen PolThanks for the update. Back to RTBC since:
1) The missing code noted in #314 was added back.
2) Tests are green again.
3) See #312 for more details on the review.
Comment #317
catch#315 looks good but unfortunately needs another re-roll.
Comment #318
quietone CreditAttribution: quietone as a volunteer commentedOK. Another reroll. The changes are to the drupal6 test fixture.
Comment #320
quietone CreditAttribution: quietone as a volunteer commentedThere was a random test failure and then I set this to retest.
Comment #321
Kristen PolThanks for the update. Back to RTBC based on:
1) Patch still applies cleanly (just checked).
2) Tests pass.
3) Was RTBC based on #316 and just needed a reroll.
Comment #323
catchCommitted 2e80fc2 and pushed to 9.1.x. Thanks!
Comment #324
Wim LeersYay!
Next up: #2814953: Migrate Drupal 7 node/user reference fields.
Comment #326
quietone CreditAttribution: quietone as a volunteer commentedPublished change record.