MigrationLookup plugin should accept zero as a legitimate input

If it is decided to not do this, then update the migration_lookup docs to state that an empty source value will throw an MigrateSkipRown exception. This is from the related issue #2911876: ids for embedded_data migration source plugin have to start at 1. And if this is done, update the docs anyway!

Change title, s/Migration process/MigrationLookup plugin/, for clarity.

I found this yesterday.

And yes, 0 is totally a valid ID, in my case, I was migrating some comments from an old Drupal version and this site had several comments made by anonymous users, Drupal saves this as 0 in the UID column making my migration to fail.

I wrote the patch and the tests.

David.

I generated the patches in the wrong place.

These should work.

Before to spend more time on this fixing the tests, who decided if we should or shouldn't do this? Migrate is already stable and maybe this will break the backward compatibility.

I already provided a patch it would be sad that if I fix the tests and in the end, the patch is rejected.

I think this definitely needs to be addressed, but you are right, there are some modules in core that do expect a 0 value to fail.

Looking at your test results offhand, the comment system uses lookups on the parent id, and expects the process to skip if the parent id is zero.

Taxonomy, on the other hand directly calls skip on empty prior to calling migration_lookup.

The truth is there is nothing in the plugin documentation that says the plugin skips empty values, I assume it only does so because FALSE, '' and NULL make no sense, and the fact that 0 is skipped is a bug. This means the comment migration is buggy and needs to be updated, but I think this behavior of skipping on empty should be fixed to skipping on invalid.

Let's try something like this.

Test bot hiccup

Hi @mikelutz

Thanks for your patch, looks great.

My main concern is that this patch changes the behavior of the Plugin and since Drupal 8.5 the migrate module has been marked as stable, so this might be considered as a break compatibility change.

So this should be reviewed by the component maintainers before to consider committing it.

I will try to contact them to see if I can get feedback.

Thanks again.

They are active, this is tagged for review, it will be looked at at the migrate maintainers meeting tomorrow. I'll make the case that this is a bug fix, not a breaking change, but they and the core committers will have to make that decision.

The fact that the behavior is undocumented and that fixing the buggy expectation in 2 migrations makes all tests pass will help.

Since we passed all tests, I cleaned up a couple small things and added documentation to the plugin to make the expected behavior clear. Also tagging for maintainer review, since this will need their signoff.

Bah, try one more time...

Discussed with @maxocub, @heddn, @mikelutz. We agreed this is a bug fix. API change is that zero throws an exception. This is an issue for things like UID = 0 or for CSV sources that use the row number of 0.

Let's add a change record and update the IS.

I wrote a draft of the change record.

https://www.drupal.org/node/3001247

Any edition/fix is really appreciated.

The initial version of this patch was meant for discussion on whether we would make this change or not. Since we are, I've polished it up a bit, and I it's ready for an actual code review/committing.

I've updated the change record to reflect that this is a bug fix. I fixed some spacing and code styling issues. I replaced code and documentation that references things being 'empty' to talk about things being valid or invalid. I also added a test that previously skipped values no longer are.

For clarity, skipOnEmpty() is now skipInvalid(). I deprecated skipOnEmpty() so any plugins extending this one won't break. I added a second change record for the deprecation.

Discussed in maintainers meeting. Protected methods don't need to be deprecated. The patch needs to be re-rolled with SkipOnEmpty removed completely, and the change record needs to be updated to match.

One module extends this plugin, and I didn't hear the name clearly, but we should submit a patch to make sure it continues to work.

It was migrate_file_to_media, but they do not call the removed function, so they are fine.

rerunning test...

Great work here. Some small nits.

+++ b/core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
@@ -106,7 +111,66 @@ public function testSkipOnEmpty() {
+    $migration->transform($value, $this->migrateExecutable, $this->row, 'foo');
+
+    // No assertions needed, as long as we didn't throw an exception.
+    $this->assertTrue(TRUE);

Can we asset on the result of the transform? That would make this comment un-needed. Maybe a assertNotNull or some such?

+++ b/core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
@@ -106,7 +111,66 @@ public function testSkipOnEmpty() {
+      [''],
+      [FALSE],
+      [[]],
+      [NULL],
...
+      [0],
+      ['0'],
+      [0.0],

Can we label these tests with an array key that explains what we are testing?

I can assertNull, since we provide no value and ask for no stub, we should get NULL back, although I feel that deserves a comment anyway.

Added labels to the the test cases.

Love it.

Testbot hiccup..

Rerolling against latest -dev to account for changes from #2981392: Comment migration corrupts data with multilingual sites

Same patch, rerolled against latest -dev again.

+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -236,19 +243,34 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
-  protected function skipOnEmpty(array $value) {
-    if (!array_filter($value)) {
+  protected function skipInvalid(array $value) {

this is allowed under this BC rule:

Particular plugin classes should not be considered part of the public API. References to plugin IDs and settings in default configuration can be relied upon however.

https://www.drupal.org/core/d8-bc-policy#plugins

Committed a92a369 and pushed to 8.7.x. Thanks!

Published change records.

Not backporting this because of the removed protected method.

While this is technically an allowed change for a protected method, we still prefer deprecating internal APIs instead of removing them, and I think it's a bit of a gray area with a change like this.

However, since Migrate is not runtime code, I decided not to mention this in the release notes.