Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Drupal provides a concat process plugin, that can transform multiple source values into a single value field, using the implode() function. It would be extremely useful to have another process plugin with the reverse functionality of exploding a single value source into multi-value fields.
Proposed resolution
Add a process plugin that can be used to explode a single value into a multiple value field.
Remaining tasks
None
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#39 | add_an-2575101-39.patch | 5.45 KB | jcnventura |
#39 | interdiff-37-39.txt | 3.52 KB | jcnventura |
Comments
Comment #2
tannerjfco CreditAttribution: tannerjfco commentedIs this a documentation issue (i.e. needs to be written) or is this an issue of additional functionality required in MigrateSourceCSV? Looking for some insight so I know how to proceed.
Comment #3
maijs CreditAttribution: maijs at Wunder commentedI don't regard this as a
migrate_plus
issue.migrate
core module doesn't provideExplode
process plugin to explode multiple values from a single line in source, regardless whether that's XML, CSV or DB. Fortunately, it's very simple to create such a process plugin. (See an excellent example in this article - Migrating to Drupal 8, seeUsers
section.)In essence:
1. Create a file
MY_MODULE/src/Plugin/migrate/process/Explode.php
2. Put in the following code:
3. In migration configuration file use the following:
Comment #4
heddnRe-assigning to new project name-space.
Comment #5
heddnSplitting by a comma or other delimiter didn't make it into core yet. I'm moving this over to the core issue queue for the time being to see if we can still fit it into things at this late stage. If not, this is the type of thing that will get added to migrate_plus. To be clear, this isn't specific to CSV migrations, it is general.
In the mean-time, this can be done in a prepare row callback.
Comment #6
mikeryanRetitling to reflect the current direction.
Comment #7
quietone CreditAttribution: quietone commented@mikeryan, should I make a patch for this with a test?
Comment #8
mikeryan@quietone: well, someone should at some point, but with a week till release I wouldn't prioritize new features for 8.0.0 - my inclination is to put this into migrate_plus for now and aim to submit to core for 8.1.0. In general, I think at this point the priority should be trying to get the more significant "needs review" issues to RTBC, and triaging open bug reports to see if there's anything we really want to try to get fixed in the next week.
Comment #9
quietone CreditAttribution: quietone commentedEnded up writing this while working on the D7 color and theme variable migrations.
Comment #11
quietone CreditAttribution: quietone commentedFixed the namespace.
Comment #12
benjy CreditAttribution: benjy at PreviousNext commentedWhy do we check value[0], that should be just $value? And then $source wouldn't be an array.
Comment #13
quietone CreditAttribution: quietone commentedYea, your right.
Comment #14
hussainwebMaking a small change. I don't see why sprintf is used there (maybe copy/paste error). And I think this needs to go to 8.1.x.
Comment #15
benjy CreditAttribution: benjy at PreviousNext commentedMigrate is marked as experimental, I don't see why this has to wait until 8.1
Comment #16
heddnI've looked over the code. It looks solid.
Are we sure we want to use var_export? Couldn't that return nasty things?
Comment #17
heddnComment #18
hussainweb@heddn: That is a general pattern in all the migrate plugins.
Comment #19
heddnIt isn't that common in Migrate. And I think we'd be better off not introducing more occurrences.
Comment #20
benjy CreditAttribution: benjy at PreviousNext commentedI don't see a problem with var_export, what do you suggest instead? Happy with print_r as well for me.
Comment #21
heddnEither is fine, but SafeMarkup::checkPlain() the values before inserting them into the exception.
Comment #22
benjy CreditAttribution: benjy at PreviousNext commentedWhy would we do that? checkPlain is for escaping a string that is going to be rendered in a HTML document, we're throwing an exception. If something else is catching that exception and printing it to the page, that's their problem.
Comment #23
heddnDoesn't watchdog grab those? So it would be printing that all to the screen for us. Does it do the checkPlain()?
Comment #24
benjy CreditAttribution: benjy at PreviousNext commentedIf watchdog is printing it to the screen, then watchdog will be escaping it.
Comment #25
heddnIn that case, I'm probably being paranoid.
Comment #26
chr.fritschWe should add multiple true, because that other process plugins in the pipeline should be able to iterate over the exploded values
Comment #27
benjy CreditAttribution: benjy at PreviousNext commentedThis would should be in the annotation, and we should add a new test if we do in-fact need it.
Comment #28
jgrubb CreditAttribution: jgrubb as a volunteer commentedhi, here's a patch based off of #26 with the annotation, I'll get to work on the test now.
Comment #29
jgrubb CreditAttribution: jgrubb as a volunteer commentedForgot to remove ::multiple() in the class.
Comment #30
jgrubb CreditAttribution: jgrubb as a volunteer commentedHey all, so the currently existing suite still passes, what else might need to have coverage here?
Comment #31
heddnAdd a unit test that tries to adjust a value into a multi-valued result and see if a following process plugin uses each individual value. Rather than on the original full string.
Comment #32
jgrubb CreditAttribution: jgrubb as a volunteer commentedHi, sorry but I have no idea what I'm doing here. I've got this going so far --
which fails --
So obviously I'm missing how to configure this test to chain processes together. I've been in the IteratorTest for most of the afternoon and am not able to tease apart how to set this up correctly.
Comment #33
jgrubb CreditAttribution: jgrubb as a volunteer commentedI'm sorry, conceptually I know what needs to be done but I have no idea how to actually implement this test (the setup of these pipelined transformations).
Comment #34
quietone CreditAttribution: quietone commented@jgrubb, for your test to pass the last item in the array 'the other' need to match. Try changing 'the other' to 'the_other' in $source. Also, instead of making a new row, you can use $this->row.
Can anyone provide a use case where multiple is needed? If so, what would a migration template look like that uses the multiple feature?
Comment #35
quietone CreditAttribution: quietone commentedComment #36
esclapes CreditAttribution: esclapes as a volunteer commentedLooking at the docs, for
::multiple()
andhandle_multiple
they have different goals.In this case I would say
::multiple()
needs to be implemented and returntrue
because explode always returns an array and chained plugins in the pipeline need to know.If the plugin should
handle_multiple
is a different question. Are we expecting an array of strings to be exploded?Patch in #26 looks like the right approach to me.
Comment #37
jcnventura CreditAttribution: jcnventura at Wunder commentedI fully agree with #36 from @esclapes. The correct code is to define multiple(), since this function outputs multiple values (and does _not_ handle them).
This patch extends #26 to include a limit 'field', so that this can be used without problem in limited multi-value fields.
I've simplified the unit test somewhat based on the simpler TestConcat, and added a simple version of the process chaining between explode and concat.
It might be useful to have a better migration simpletest where we can define a full migration, and chain these values properly, but that's outside the scope of this issue, since that simpletest would focus on the proper handling of the multiple() = TRUE and handles_multiple plugins.
Comment #38
benjy CreditAttribution: benjy at PreviousNext commentedDo we need a stub class,
public function __construct(array $configuration, $plugin_id, $plugin_definition)
the parent constructor definition looks simple enough that we could just pass in the values and set the config at the same time? See ExtractTest for example.Rest looks good, RTBC once we've removed the stub class for testing.
Comment #39
jcnventura CreditAttribution: jcnventura at Wunder commentedRemoving the stub classes as per #38 + minor drupalcs warning.
Comment #40
benjy CreditAttribution: benjy at PreviousNext commentedGreat, looks good to me.
Comment #41
jennypanighetti CreditAttribution: jennypanighetti commentedThis is exactly what I need for an upcoming import.
If I want to implement this before this gets into dev (or, not using the latest dev), do I just need the latest patch in #39?
Comment #42
quietone CreditAttribution: quietone commented@jenstechs, yes. Make sure your version is the same as shown at the top of this issue.
Comment #43
alexpottCommitted 4e39681 and pushed to 8.0.x and 8.1.x as migrate is experimental and this new feature does not introduce any risk to non-experimental code. Thanks!
@heddn btw exception messages are not supposed to use the markup or translation tools at all. Anything that displays them has to (and does) handle this.
Comment #47
heddnExplode had its own docs already, where the configurable delimiter is documented. Migrate source CSV had a configurable delimiter also; as it's underling php CSV also supports it. Please check out on their respective handbook pages
Comment #48
RKopacz CreditAttribution: RKopacz commentedHi @heddn thanks for this info. It answers my question which I asked here. My only question is whether, in the config file, to use the syntax recommended in comment #3 above in the config file?.
Comment #49
maxocub CreditAttribution: maxocub commentedWrong version.