Fields hidden in some view modes abort migration (D6)

The fix makes sense if the fields default to hidden in D8 which is what the IS says.

Can we add a test for this?

Tagging as novice, if someone wants to work on this feel free to ping me in #drupal-migrate for some assistance.

I'm going to jump in on this one. So this fix is good to go if all the fields are hidden @benjy said. What does IS stand for? Trying to determine if you're asking for a conditional check inside the function or a unit test to review the patch.

EDIT: After further reading I understand what context you guys are using the word 'test' in. I should have this done in the next day or so.

@th3m0d: Did you have a chance to work on a test for this?

Thanks.

Not sure if this is the right fix but here is a re-roll around The Great 2015 Migration Migration

another reroll

That's weird... I wonder how we got stuck on 8.1.x

I can't reproduce the error. But then, the views I made aren't migrated at all. No error messages on screen nor could I find any in log files.

Do you mean select 'exclude from display' when you say "Set some fields to be hidden in some of the view modes"?

I'll make it a priority to provide test date.

@KarenS, thx for the explanation. I thought I had it wrong.

The fix looks good, but we do need to add this scenario to the D6 dump and test it.

Is it an issue in D7 as well?

This happens in D7: "Missing formatter: The 'text_hidden' formatter used in 1 view modes for the field_myfield field is not available, these displays will be reset to the default formatter."

I can't reproduce this in D6 nor D7.
When I set certain fields as hidden in some view modes (full, teaser), I get no errors upon migration to Drupal 8.
On my Drupal 8 install, all fields that have been marked hidden in D6 / D7, are still in the "hidden" region, and thus not rendered on node view.

Could someone who still has this problem elaborate on the steps to reproduce this?

OK, I now realize that this issue is about hidden fields. I was talking about excluded fields (the excluded column with a checkbox). Sorry.

AFAIK Drupal 7 doesn't have that excluded checkbox?
I tested the checkbox in D6, and the result is the same as with a hidden field: in D8 the field gets moved into the Hidden region of the view mode.
So not sure if this issue is still relevant at this point.

A field that was excluded in D6 doesn't get moved into the Hidden region in D7. Maybe I should add a new issue for that.

Can't seem to "add test" on the patch, so forcing a retest (which I'm betting will demonstrate a reroll is needed...)

Guess it's too old - re-uploading (committers - no credit for me on this!).

Looks like we need to add some tests. While that *could* be easy, writing tests isn't generally super simple. Removing the novice tag.

This definitely needs tests. It shouldn't actually be too tricky -- just load the resulting D8 entity_view_display entity and ensure that the hidden field (or "component" in D8-ese) does not exist in the view display.

Testing if needs re-roll. Some offset found. Patch updated.

curl https://www.drupal.org/files/issues/fields_hidden_in_some-2498291-11_0.patch | patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2061  100  2061    0     0   3366      0 --:--:-- --:--:-- --:--:--  3362
patching file core/modules/field/src/Plugin/migrate/source/d6/FieldInstancePerViewMode.php
Hunk #1 succeeded at 28 (offset -5 lines).
patching file core/modules/node/src/Plugin/migrate/source/d6/ViewMode.php
Hunk #1 succeeded at 21 (offset -5 lines).

Changed an existing field to hidden and added a test for that in MigrateFieldFomatterSettings.

@quietone Is it possible for $field_row['display_settings'][$view_mode]['format'] to be unset? That's the only issue I can see. Otherwise this is ready for RTBC (after it has been re-rolled).

Also removed the "Needs tests" tag because #39 includes them.

Re-rolled.

Looks great. Nice and simple, and there is test coverage. I just have two tiny concerns:

1. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldFormatterSettingsTest.php
   @@ -21,6 +21,19 @@ protected function setUp() {
   +    $this->assertTrue(is_null($component));
   

   I'd prefer to use assertNull here.

2. +++ b/core/modules/node/src/Plugin/migrate/source/d6/ViewMode.php
   @@ -21,7 +21,7 @@ protected function initializeIterator() {
   +        if (isset($field_row['display_settings'][$view_mode]) && $field_row['display_settings'][$view_mode]['format'] != 'hidden'  && empty($field_row['display_settings'][$view_mode]['exclude'])) {
   

   This code is repeated in FieldInstancePerViewMode. If one of these classes extends the other, can this logic be a helper method, with a doc comment to explain exactly what it does? Also, a nitpick: there is an extra space before the final &&.

@phenaproxima can you give more details on how you see the helper method working, please? I've got plenty of ideas, but they all seem to have flaws. e.g. FieldInstancePerViewMode could extend ViewMode, but then we are adding calculateDependencies() to FieldInstancePerViewMode. Another idea was to transfer many of ViewMode's methods to ViewModeBase, but I suspect that is not a good idea even though these are the only two classes that extend that.

Oh -- I only meant that we should have a helper method if one of the classes already extends the other. If that is not the case, then I'm OK with duplicating the code, as long as there is a comment that is a) duplicated in both places; b) explaining what the if statement is doing; and c) has a @see that mentions the other place where the same if check exists.

1. Used assertNull()
2. Added comment to if().

Excellent! Full stream ahead...

My other issue seems to conflict with this one where I went with !empty() @Jo Fitzgerald could you review and feel free to close mine if the change isn't needed with what you've changed here.
#2854314: D6 FieldInstancePerViewMode has a default viewmode with no settings

@joelpittet I've taken a look at your ticket, but I don't think there is any conflict with this one. Have a look at my patch(es) for more details on my thoughts: https://www.drupal.org/node/2854314#comment-11994882

They were just failing to apply with composer patches between each other, didn't diff the diffs just wanted to show they were touching the same lines with empty check

Committed and pushed acc5c66 to 8.4.x and 3610a36 to 8.3.x. Thanks!