Add cvsrownum to the source CSVFileObject

Interesting 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.

@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.

I 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...

@heddn Do you mean something like this?

Yes, but I would (for BC reasons) default to not include the field but only add it if the field is configured.

This adds an optional record number field. It is only added if the field is specified during configuration.

+++ b/src/Plugin/migrate/source/CSV.php
@@ -229,7 +231,21 @@ class CSV extends SourcePluginBase implements ConfigurableInterface {
+    if (array_key_exists('record_num_field', $this->configuration)) {

Instead of doing a check of existence, we could set some default values in ::defaultConfiguration(). That would simplify this a little. Love the tests.

That is will be a simple change to make and will clean it up. I avoided the default configuration due to comment #8 above.

Here it is using the config instead.

Here it is using the config instead.

Sorry I missed a few things on the initial review. This is really shaping up well. Thanks for your contributions.

1. +++ b/src/Plugin/migrate/source/CSV.php
   @@ -130,6 +134,14 @@ class CSV extends SourcePluginBase implements ConfigurableInterface {
   +    // If set, "create_record_num" must be either TRUE or FALSE.
   +    if (isset($this->configuration['create_record_num']) && (!(is_bool($this->configuration['create_record_num' ])))) {
   +      throw new \InvalidArgumentException('The configuration "create_record_num" must be either TRUE or FALSE.');
   +    }
   +    // If set, "record_num_field" must not be empty.
   +    if (isset($this->configuration['record_num_field']) && empty($this->configuration['record_num_field'])) {
   +      throw new \InvalidArgumentException('The configuration "record_num_field" must not be empty.');
   

   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 if create_record_num has a value.

   Something like the following would probably suffice:

       // If "create_record_num" is specified, "record_num_field" must be a string.
       if ($this->configuration['create_record_num'] && !is_scalar($this->configuration['record_num_field'])) {
         throw new \InvalidArgumentException('The configuration "record_num_field" must be a string.');
       }
   

2. +++ b/src/Plugin/migrate/source/CSV.php
   @@ -229,7 +243,18 @@ class CSV extends SourcePluginBase implements ConfigurableInterface {
   +    /**
   +     * The record_num_field is only created if create_record_num was set
   +     * in the configuration. If it was specified, but no custom field name
   +     * was given, the default field name is record_num.
   +     */
   

   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...

3. +++ b/src/Plugin/migrate/source/CSV.php
   @@ -229,7 +243,18 @@ class CSV extends SourcePluginBase implements ConfigurableInterface {
   +    if ($this->configuration['create_record_num']) {
   

   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.

4. +++ b/src/Plugin/migrate/source/CSV.php
   @@ -229,7 +243,18 @@ class CSV extends SourcePluginBase implements ConfigurableInterface {
   +        $record[$this->configuration['record_num_field']] = strval($record_num++);
   

   PHP casts to string for us, no need to cast with strval.

Really, 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.

Thanks 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.

I like using header_offset as the basis for the counter.

I made those changes on commit. Glad you agreed.