Add UI to alter column_names for CSV source files

Here is a start and it was working locally.

However, I don't know forms and there is no way to get there from the UI, you need to enter the URL, /admin/structure/migrate/manage/{migration_group}/migrations/{migration}/source_csv, which I found easiest to do by navigating to the View->Source for the migration and then modifying the URL. Obviously, this needs to be fixed. Ideas? Since I don't know forms a patch would be best or, at least, an example.

This does not change the migration in config, it does a hook alter, so the changes are not permanent.

This add links so that you can get to the new edit form. Still needs work to fix the failing tests and it should only show the new edit form for CSV sources.

Add a fix for the test as well as comments and code standard fixes.

The test is fixed by adding a if in migrate_plugins_alter to check that the stored value exists before using it.
+  if (isset($store) && $store->get('source_configuration')) {
+    foreach ($store->get('source_configuration') as $key => $value) {
+      if (isset($value['updated'])) {
+        $migrations[$key]['source']['column_names'] = $value['updated'];
+      }

If the source plugin is not of class CSV then it is not editable and a message is now displayed. Testing found test code lying about and typos, all that is fixed now. And the SourceCsvForm:buildForm() is reworked so error cases are checked in the beginning.

Add another check in the plugin alter.

Started to add a test but stuck on schema errors.

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for migrate_plus.migration.csv_source_test with the following errors: migrate_plus.migration.csv_source_test:source.path missing schema, migrate_plus.migration.csv_source_test:source.header_row_count missing schema, migrate_plus.migration.csv_source_test:source.enclosure missing schema, migrate_plus.migration.csv_source_test:source.keys missing schema, migrate_plus.migration.csv_source_test:source.column_names missing schema

Update the IS.

Ready for initial review and testing.

I switched to using the existing test migration in migrate_tools and just changing the source plugin and process. I don't yet know why the previous one failed. I might look at it one more time.

This patch adds a functional test and in making the test I discovered two errors. They aren't fixed yet, I want to see the failures here.

Perhaps it needs @requires.

The new test isn't running on the testbot. Anyone know why?

1. migrate_source_csv added to composer.json
2. Yes, I was just responding to the errors. What is a better way?
3. Added +module_dependencies
4. Fixed and learned how to do that.
5. Haven't changed this because I prefer a helpful error message to access denied for these two cases.
6. No, because all the source configurations use the same key 'source_configurations'. Maybe that should change.
7. Maybe I'm not understanding this, when no length is given fgets will read until it reaches the end of the line.

2. OK The alter runs after the cache clear. During test of the plugins alter I need to drush cr, then do a drush ms for the alter to run and debug it, if necessary.
5. I'll get back to you on this. This patch makes a small change, adding a module dependency in routing.yml and removing the corresponding code from the form.
6. Yes, it should change. It was just the first go. I want to improve this next.
7. NP

1. Here, $migration is the plugin id, it isn't bringing in the object.
2. Fixed.

This patch fixes the things that came up while I was writing the test. The test runs locally but not yet on the testbot but when it does hopefully it will pass.

Don't let the green fool you, the new test still isn't running.

#22.6 This changes to the key used for the store to the plugin_id. Passes tests locally so seems to be fine. It much simpler and the comments updated as well.

Now I remember the reason all the change migration information was stored in one key, source_configurations. It made it easy at the time for the alter to find the migrations that were changed. It would get the 'source_configuration' from the store, which had a key for each migration changed. Now that the changes are stored by migration_id, the alter has to loop through all migrations and check if the id is in the private store. Instead of that there is now a simple array of the changed migrations in the store.

Changed the index 'updated' to 'changed' throughout because I prefer it.

Still to do is #22.2 and #22.5.

22.5 Should be fixed in this patch.

@MegaChriz, thanks for stopping by and commenting. On a first look, having a source and a target column seems a better way to present this information. I haven't read through the UI designs issue yet, thanks for the link.

This patch has quite a few changes:
The test for the currentUser in migrate_tools.module has been removed. I guess the addition of the Access checking has eliminated the problem.
The URL is now '/admin/structure/migrate/manage/{migration_group}/migrations/{migration}/source/edit'. Before it didn't include 'edit'.
Fixed a typo in the name of the migration_changed key
Reworked the test module migration to work for a taxonomy vocabulary destination so that a migration can be executed and tested
Execute the migration twice in the test to verify different results are optioned when the aliases are changed.

Yes, lets get this in and move the UI improvements to a follow up.

The work to do here is to get the new test running on the testbot and review, no more features/additions planned here.

To start, what is wrong with the test?

Follow up made #2954082: Improve CSV alias edit form

@heddn, thank you.

The form needs the \SplFileObject object for the source CSV file in order to determine the things like the header row count etc. This is already being calculated in the CSV:setupFile protected method. If that was made public then the form would not have to duplicate that setup code. Is there any reason not to make CSV:setupFile public? It should be noted that setupFile is called from initializeIterator() which is public, but that won't work as it is not guaranteed to return an SplfileObject. In the case when the source plugin uses yeild it returns a Generator.

Can the form be used to pass the SplFileObject?

edit: add initializeIterator

A getter for the file in CSV class will work.

Getting closer now. Now gets the file with all the properties setup via the new getter in migrate_source_csv. I also added another try catch in the cancel function, to match the existing one, on this->store->set. Not sure how to handle those execptions. Maybe just display the returned error?

Change the version of migrate_source_csv in the test dependency to -dev. Add display of error message if TempStoreException is caught in the cancel method.

Change composer.json to use migrate_source_cs 2.x-dev

Remove the carat

Yay, the test is running now. Boo, I found a typo.

The changes since #40 are to get the file properties initialized and then get the SplFileObject for the file. This is now down with the following code.

    // Get the source file after the properties are initialized.
    $source->initializeIterator();
    $this->file = $source->getFile();

And to display any error messages if the actions on the store throw an exception during when the cancel button is clicked.

And to use the dev version of migrate_source_csv

this should be tested by someone other than me.

@irinaz, thanks for the offer!

Yes, apply the patch. And you will need an existing migration that uses a CSV file for import. You may have one from a project or the example here should work, https://github.com/heddn/d8_custom_migrate. The process is to run the migration, verify the results, rollback the migration, change the column assignments, run the migration, verify the results. We need to be sure that the changes made to the column assignments really work.

I wrote this in steps to help make sure I didn't miss anything.

1. Go to the /admin/structure/migrate execute the migration.
2. Verify on the site that the migration and that the correct data is displayed
3. Go to the /admin/structure/migrate rollaback the migration.
4. Go to /admin/structure/migrate click on the name of same migration.
5. Click on the 'Edit' tab
6. Click on 'Source'
7. You should now be able to change the column aliases for the fields in that migration. That is you can change it so that the migration uses column 2 this time for field A instead of column 1 for field A.
8. Make some changes to the columns assignments
9. Click Submit
10. Then, as before, run the migration.
11. Verify on the site that the migration and that data displayed is from the changed columns.

I hope that make sense. If you get stuck, ask here.

@irinaz, sorry I ddn't see this earlier. Ah, looks like a missing step which is to upload the csv. I am working from memory but on, /admin/structure/migrate, there should be a button in the upper right to upload your source file. Upload your source file first and then it should work.

Good luck!