trackLastImported YAML key to enable tracking the last import date of a row is undocumented and confusing.

Add `track_last_imported: true` to the source: property of your migration.

The track last imported is a property of the migration and should be set there. And, in fact, it is but the name is trackLastImported. If that is used it will work.

id: test
trackLastImported: true

I am not sure if the property name should change or not. It is used as a variable name in a few places which I guess adds to the confusion.

(9.2.x)$ grep -ri track_last_imported core/modules/migrate
core/modules/migrate/src/Plugin/MigrationInterface.php:   * @param bool $track_last_imported
core/modules/migrate/src/Plugin/MigrationInterface.php:  public function setTrackLastImported($track_last_imported);
core/modules/migrate/src/Plugin/Migration.php:  public function setTrackLastImported($track_last_imported) {
core/modules/migrate/src/Plugin/Migration.php:    $this->trackLastImported = (bool) $track_last_imported;

This patch add tests to show that Sql.php save a timestamp to 'last_imported' when tracking is enabled and that 'last_imported' is empty when it is not set. Also, added a test of getting the trackLastImported to MigrationTest.

@swirt, yes, the convention is snake_case and I guess it should change. If someone out there is using trackLastImported though we have to maintain BC and I don't see anything in the deprecation policyfor class properties. I was planning on asking in Slack or the next migrate meeting.

Reroll for 9.1.x for those of us who need it.

@natedouglas, thanks for the patch for 9.1.x. If the patch is working for you add a comment explaining the circumstances. It will help move the issue towards RTBC is there a report that it works. Thx.

Updated Patch.

Let's try that again

I promise I'm not dumb. Another reroll of the patch with fixed spelling mistakes.

Thanks folks, so it looks like the final outcome here is that we're adding new tests to support trackLastImported - so can we get an issue summary and title update to support the resolution

No, That is not the final outcome, I think we will go ahead and add support for snake case and deprecate the camel case and add a bunch of documentation.

@indytechcook, please do not RTBC your own patches. You may set your own patches to 'needs review', and a community member will review them and set them to reviewed and tested by the community if appropiate. This issue still needs work.

And now that I went through updating all that, I must ask.. why is this even a configuration option, why don't we just track this all the time? I don't think there is a performance issue, to add one more column (that already exists) into a database insert/update (which already exists)

The feature is completely undocumented, and the data is inaccessible without looking into the database, or using migrate tools, so I don't see any BC implications.

At worst, if there actually is a performance issue, It seems like something like this should be opt-out, not opt-in. This seems like pretty useful information for people doing custom migrations.

Thoughts?

@mikelutz, thanks.

This patch does steps 1, 3, and 5 in the Remaining steps. There is no interdiff because this is a new approach.

todo:
2) Deprecate use of trackLastImported and standardize on using snake case for migration configuration keys. I don't see anything in href="https://www.drupal.org/node/2856615">Drupal core deprecation policy about how to deprecate a class property. I guess I am missing something?
4) Create a follow-up (novice?) ticket to improve documentation in the Migration class in general, giving more details about all the supported keys.

I was curious about where this might be used. I found that there are two uses of setTrackLastImported(TRUE); in contrib. And then found an older related issue, which was closed as works as designed. But now we are dealing with all the confusion.

Follow up made #3226401: Add documentation of migration yml files and it has a patch available for review.

That just leaves whatever needs to be done remove the property.

Deprecating a class property requires implementing __get and __set magic methods that detect when people attempt to write/read from the old property and then triggers an error and writes/reads the new property instead.

Technically we would also default the new property to false while triggering an error if it isn't set in a migration and then switch the default to true in D10, but I really don't think we need a shim for the behavior change here. The change is not breaking, we are simply storing additional information with each run. If trackLastImported didn't already exist as an opt-in then we would have absolutely no problem adding this tracking as an opt-out feature in any minor release, and given the lack of documentation around trackLastImported, I'm of the opinion that it's use was confined to a few people who turned it on because they happened to stumble across the fact that they could. I think for BC purposes we should make sure we turn it off and trigger an error if anybody explicitly set trackLastImported to false, but for anybody that did not use the key at all, I'm content to turn this extra tiny bit of data storage on. Again, the column already exists, we should just use it.

@mikelutz, thanks.

I didn't do anything about deprecating the property because there is I can't find anything in Drupal deprecation policy about it. Nor could I find an issue on the topic. Anyone know of any?

This patch changes the default to FALSE and includes a first attempt at the deprecations, with tests. The deprecation message does need a link to the still to be created change record.

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -142,6 +157,13 @@ class Migration extends PluginBase implements MigrationInterface, RequirementsIn
+  /**
+   * Track time of last import if TRUE.
+   *
+   * @var bool
+   */
+  protected $track_last_imported = TRUE;
+

We don't actually have to change the name of this variable, in fact, we shouldn't as snake case is appropriate for an object member. This whole migration class loops through the configuration and sets an object member for whatever happens to be there, which we need to fix anyway in a follow up issue, as future versions of php are going to complain about that anyway. Just default ::trackLastImported to TRUE, in the constructor, check for $configuration['trackLastImported'] if it's set, throw a deprecation error. if it's set and false, set ::trackLastImported to false and throw a deprecation error. If $configuration['track_last_imported'] is set and false, set ::trackLastImported to FALSE. unset both configuration keys afterward if they exist. None of the rest of the logic should have to change. We want to deprecate the configuration key, but we don't need to touch the variable name.

@mikelutz, thanks for the review.

I admit I had trouble unpacking #28 but I do get the intent, which I agree with. However, I don't think that the default value should change from FALSE to TRUE. With that in mind I tried to implement a simpler approach and thus no interdiff. One question that came up was should we handle the case where both trackLastImported and track_last_imported defined?

random fail, Drupal\Tests\quickedit\FunctionalJavascript\QuickEditFileTest::testRemove

I'll take look when I get into the conference room. Why don't you think we should just enable this by default? I'm still of the opinion it's a non breaking change that would improve developer experience with near zero performance impact, but I'd love to understand any objections. I may very well be missing something.

Just seems wrong to change behavior. But your right that it is a non breaking change. This should be the patch you wanted in #28.

I didn't realise this feature existed. I was looking in the migrate map tables for something unrelated and spotted that the last import timestamp was 0; I agree with #20 and #33 that this should be enabled by default, and I'm not even sure why we need a flag to control it.

I really agree with longwave that we don't need to let people opt-out from this. We could definitly simplify the code if we just remove the option completely. @quietone, @heddn, @benjifisher - Can anyone think of a good reason to keep an opt-out flag here? The only justification I can think of is performance, but my gut says that is negligible even on large data sets, and for the 99% of people that would leave this in, losing the processing around the flag would technically be more performant, (though again, negligible). I'm really of the opinion that unless I see a benchmark test showing significant performance degradation by adding this column that we should just remove the flag completely. Again, the column already is allocated as a non-nullable 10 digit integer, the row insert/update happens whether the extra couple bytes of data for that column is included or not. Mysql has to write SOMETHING there anyway at least on insert, whether that's 0 or a real number doesn't really matter.

I can't think of a good reason not to track it. Feedback posted below.

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -38,12 +38,15 @@
+ * - track_last_imported: (optional) If set to false a timestamp will not be
+ * added to the row indicating the time this row was imported. Default to FALSE.

If set to true, a timestamp of the date the last row was imported will be tracked. Defaults to true.

If we are not going to use it then I guess we should deprecated MigrationInterface::isTrackLastImported, \Drupal\migrate\Plugin\MigrationInterface::setTrackLastImported and \Drupal\migrate\Plugin\MigrationInterface::getTrackLastImported.

+1 for removing the option and always tracking the timestamp.

I agree that the performance difference is likely to be negligible. If someone has a reasonably large migration to test (one that takes at least 5 minutes to run) it would be reassuring to see some timing data with and without the trackLastImported option set.

The only question I see is how hard we have to work to deprecate the option. I see that #28 recommends throwing deprecation errors if either version (snake_case or camelCase) of the option is set. Do we need to do that? Do we throw any sort of error if a migration has random, unsupported keys in the YAML? I think not, since I sometimes copy YAML files from a configuration directory (supported by migrate_plus) to mymodule/migrations/ and I do not see any warnings about migration_group and other keys.

I will play devil's advocate. These two hypothetical objections are not serious.

• Privacy: We are storing more information. Maybe a timestamp somehow identifies some of the other data in the migration.
• BC: someone wrote code that relies on the 0 saved in the database.

This patch deprecates trackLastImported, isTrackLastImported(), setTrackLastImported(), and getTrackLastImported().

No interdiff because this is a different solution.

+++ b/core/modules/migrate/src/Plugin/Migration.php
@@ -358,6 +363,12 @@ public function __construct(array $configuration, $plugin_id, $plugin_definition
+    // Set trackLastImported independent of the following  mergeDeepArray().

This comment is out of place, probably copy/paste error - not sure we need a comment here at all.

Otherwise this looks ready to go.

Sometimes I think I am famous for copy/paste errors because I do it so much.

All fixed.

Super close here. I just noticed we are deprecating in 10.0.0 though, I think we need to target 10.1.0, right?

Updated patch #43 by addressing #44, please review it.

#45 looks great to me.

I queued a test of #45, but it is not being re-tested regularly. Maybe another comment while still RTBC will wake up the testbot.

Patch is still not retesting. This time let's try a uploading the patch again.

The patch in #45 (re-uploaded in #48) needs a reroll. I am attaching a new patch and a diff.

This was an easy reroll, so I am leaving the status at RTBC.

Easy means that I applied the patch to a commit on the 10.1.x branch, then rebased on the HEAD of that branch. I let Git do the work. Then I generated the new patch.

It looks as though the conflicting commit comes from #3181778: [w/c September 17th] Replace t() with $this->t() in all plugins. The title of that issue looks consistent with the attached diff.

Adjusting issue credit and fixing issue summary to agree with the solution.

Committed eefe3a8 and pushed to 10.1.x. Thanks!

Fix some typos.