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.
Sometimes the data has just nothing to grab it from, no ids, emails, or reliable unique source key to use, in those cases, having just the rownum helps to get by.
Migrate in D7 had it as per https://www.drupal.org/node/1701096
Comments
Comment #2
pcambraI managed to add it when the current record is calculated, not 100% sure is the best place for this.
Comment #4
heddnInteresting idea. I don't think this is something though I'd want as a feature added to the module someone could always modify the CSV and row numbers get off. If someone runs into this, it is much better to use all the values in the CSV as the key or some some subset that allows the data to be identified as unique.
Comment #5
DuaelFr@heddn I have another use case where this could be useful:
I have to import a taxonomy from a CSV file. I want those terms to be sorted as theyr are in the file. I could totally use the row number in to fill the weight of the term.
The Migrate Spreadsheet module has such an option and it's really helpful.
The cost of this feature is really low and could help people somehow, but I agree that using this as the key should be strongly discouraged, even if it could be the only option if other columns have too big values to fit in the index, as told in your documentation.
Comment #7
heddnI wonder if we could add a config option that would add the row number. Then add it as a row number property somewhere around https://git.drupalcode.org/project/migrate_source_csv/blob/8.x-3.x/src/P...
Comment #8
DuaelFr@heddn Do you mean something like this?
Comment #9
heddnYes, but I would (for BC reasons) default to not include the field but only add it if the field is configured.
Comment #10
csmdgl CreditAttribution: csmdgl as a volunteer and commentedThis adds an optional record number field. It is only added if the field is specified during configuration.
Comment #11
heddnInstead of doing a check of existence, we could set some default values in
::defaultConfiguration()
. That would simplify this a little. Love the tests.Comment #12
csmdgl CreditAttribution: csmdgl as a volunteer and commentedThat is will be a simple change to make and will clean it up. I avoided the default configuration due to comment #8 above.
Comment #13
csmdgl CreditAttribution: csmdgl as a volunteer and commentedHere it is using the config instead.
Comment #14
csmdgl CreditAttribution: csmdgl as a volunteer and commentedHere it is using the config instead.
Comment #15
heddnSorry I missed a few things on the initial review. This is really shaping up well. Thanks for your contributions.
The issets aren't needed, because the default values now specify a default value. And w/ PHP's truthiness, we can just check if there is a value in
record_num_field
. Boolean isn't really needed.For
record_num_field
, we only want to make sure it is_scalar ifcreate_record_num
has a value.Something like the following would probably suffice:
This type of comment block isn't usually used for inline comments. It is only used for docblocks on functions/methods. For comments inline to code, single line comments are preferred. See https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...
There's no harm w/ assigning a value of 1 to
$record_num
and not using it. Adding the conditional adds to the cognitive dissonance. You could probably get away without adding it and just assigning to it always.PHP casts to string for us, no need to cast with strval.
Comment #16
csmdgl CreditAttribution: csmdgl as a volunteer and commentedReally, really appreciate the feedback. Thank you!
For #1, I think it also needs to check for an empty string so that we don't get a field without a name.
Comment #18
heddnThanks for your contributions. I made a few cosmetic changes to lengthen out variables names and default to use
header_offset
as the basis for the counter.Comment #19
csmdgl CreditAttribution: csmdgl as a volunteer and commentedI like using header_offset as the basis for the counter.
Comment #20
heddnI made those changes on commit. Glad you agreed.