Undefined index exclude

+++ b/drupal/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ViewMode.php
@@ -26,7 +26,7 @@ protected function runQuery() {
+        if (isset($field_row['display_settings'][$view_mode]) && !isset($field_row['display_settings'][$view_mode]['exclude'])) {

This changes the logic, previously we were testing whether exclude was true/false where as now we're testing if the field is set. if exclude was true, this code would be wrong.

Re-uploading the patch to trigger the bot, I wonder if our tests have this covered.

Wow, I uploaded the wrong patch last time :/ Try again...

fixed patch

I'm getting inconsistent test results, but here goes.

I think the first part was only meant to check if the $view_mode was set. I'm not entirely sure if 'exclude' is always set or whether it's true or false we could check that and then write the code based on that assumption otherwise we could handle all instances:

 if (isset($field_row['display_settings'][$view_mode]]) && (!isset($field_row['display_settings'][$view_mode]['exclude']) || !$field_row['display_settings'][$view_mode]['exclude']))

Right, see your point. I was revisiting what php does when testing an unset variable in arrays and well, got so busy looking at the trees I missed the forest, as it were.

I too don't know if exclude is always set, but is when it is, it is 0 or 1.

The changed line is 81 characters. According to Line length and wrapping that seems to be allowed in this case. Let me know if it should be changed.

Oh dear. Maybe this will be third time lucky.

Well this now covers both the possibility it's true/unset and true/false. I'm OK with that.

Can we update a test to cover the new "unset" case?

It seems like a good idea to add a test. But I'm not currently able to do that.

I've got the time if anyone is willing to help me learn it.

To test this, look at the "Dump" file that provides the table "content_node_field_instance". From there, remove the exclude setting entirely from the display settings on of the view modes. Then we should be able to assert that the view mode is still imported. It's likely that we already have tests that assert this.

@benjy, thx. And here's a patch.

Can you tell me why only the view_mode 'preview' is tested in ViewModeTest.php and not the other ones?

No I can't think of any good reason why we're only testing view_mode 'preview' and no others in the unit test. Likely the view modes were added for testing something else and then the unit test never got updated. That's out of scope for this issue.

Patch looks good. Could you please post a copy of the patch without the new fix so we can confirm that the test fails?

@benjy Is this what you mean?

The attached patch will cause 2 failures, one in ViewMode.php about the undefined index and another from MigrateFieldFormatterSettingsTest.php. Then, apply 2348447-23.patch and the tests pass.

Yes that is what i mean. Thank you.

Issues is RTBC as long as #25 fails which it should.

#23 is RTBC

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/FieldInstancePerViewMode.php
@@ -31,7 +31,7 @@ protected function runQuery() {
+        if (isset($field_row['display_settings'][$view_mode]) && isset($field_row['display_settings'][$view_mode]['exclude']) && !$field_row['display_settings'][$view_mode]['exclude']) {

+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/ViewMode.php
@@ -26,7 +26,7 @@ protected function runQuery() {
-        if (isset($field_row['display_settings'][$view_mode]) && !$field_row['display_settings'][$view_mode]['exclude']) {
+        if (isset($field_row['display_settings'][$view_mode]) && isset($field_row['display_settings'][$view_mode]['exclude']) && !$field_row['display_settings'][$view_mode]['exclude']) {

Don't we want to create a view mode if exclude is not set?

So this could be just empty($field_row['display_settings'][$view_mode]['exclude']), no?

@alexpott, thanks for the review. I believe you're right.

@quietone, lets add a test that verifies the new view mode you added to the dump is migrated. Right now, that should fail (post a patch to prove it).

Then, lets update this logic as suggested by alexpott and hopefully the new and existing tests should pass.

OK. Here are the tests, which should fail.

And a patch

OK. Nice to have the errors confirmed, I've been getting inconsistent results.

Them errors are related to the tables for the new fields you added been missing in the dumps. You should be able to copy another field to create those tables.

@benjy, thx.

Once again with the new tests that will fail.

And a new patch

Forgot to upload the patch.

Hmm, that test passes when I run it by itself.

New tests that will fail.

The change is to add "exclude => 1" for the print view mode in field_test_exclude. That will allow the existing tests to pass and still allow the testing of migrating a field where exclude is not defined.

And the patch.

Great job @quietone. I checked the existing tests and we already had coverage for testing the lack of display modes that are excluded in the dumps.

The only confusing thing here is that we have a field called field_test_exclude which is actually to test exclude being unset but I think we can let that through, hopefully the dumps will be auto generated soon once #2348875: Improving our dump files lands.

Thanks @benjy. I'm glad we agree about the existing tests.

And I wasn't too happy with the name either. And now that I read your comment, I've changed field_test_exclude to field_test_exclude_unset and attached a new patch. I don't mean to make more work for anyone, but the name change does make it clearer what is being tested. So, for me, it is worth changing the name.

Great, still RTBC.

Reroll

Back to RTBC

This issue is a unfrozen change (migrate) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 4e06a75 and pushed to 8.0.x. Thanks!