Regression: Fields missing from translation overview since "Clarify handling of field translatability"

Noticed while working on #1920876: Add a tiny bit of state magic in the image.module and hide the Title and Alt groups when the related form items are disabled

issue probably needs a new title once we figure out what is going on.

missing fields are:
Comment settings
File (image)
Tags

I know adding a new simple text field does show.

Yes, fields added via the UI do show.

this does make the tags and image file show.
looking into how to change the setting for comments.

@franSeva is also working on this in irc

@Gábor Hojtsy helped me in irc find where the comment setting was coming from.

reinstalling with these settings does make the fields show in the overview.

attaching new patch and interdiff.
Still waiting for the test from @franSeva

===============

However, I'm still concerned that the config is being saved for instances with translatable false.
This is concerning because 1) it should be true.
And, if it is false... then why are they being shown as translatable (checked) in the overview.

I installed the config_devel module
and then in
sites/default/files/config_devel
ag "translatable: false"
gives

field.field.user.user_picture.yml
28:translatable: false

field.instance.block_content.basic.body.yml
16:translatable: false

field.instance.comment.comment.comment_body.yml
16:translatable: false

field.instance.node.page.body.yml
16:translatable: false

(user picture is fine. As I understand it... it should be false.)

[edit: added...}

From the db, at first I thought the data was binary, cause phpmyadmin said it was, but when I saved the "bin" file, and opened it, it was readable text with:
s:12:"translatable";b:1;
b must be boolean
and...
0 is false
1 is true
so in the db, it's true

why when it writes out the yml is it false?
I tried tracking down how yml gets saved... and haven't yet had luck.

I'll check back in on this later, especially to see if anyone has ideas about the config saving wrong.

side note I should open an issue about:
I think that translatable property should be renamed to supportsTranslation (and method isTranslatable() to supportsTranslation() ) for the field definitions.
and we keep translatable and isTranslatable() for the instances.
------

Seems to me that via the UI:
creating, for example, an image file field,

1) that that field definition is translatable true (supportsTranslation), because ... image file fields by default are/do. (and nothing can change whether it supports translation or not)

and

2) that the specific instance of that field created via the UI is initially translatable false, because it has yet not been set to be translatable.

3) but that once the instance *is* set to be translatable, that exporting the config for that instance, should include the fact that it is translatable. (it's translatable in the database)

hmm...
How are other instance settings saved? Like the cardinality, or default value, or allowed values? seems like translatable would be similar

Sorry, I think I didn't understand what "instance" meant.

field.instance.comment.comment.comment_body.yml

uuid: 8902af40-2297-44ef-b52a-4271948de4cc
langcode: en
status: true
dependencies:
  entity:
    - comment.type.comment
    - field.field.comment.comment_body
id: comment.comment.comment_body
label: Comment
field_uuid: f86f0984-81c3-4120-af01-1e2b8d64cbb0
field_name: comment_body
entity_type: comment
bundle: comment
description: ''
required: true
translatable: false
default_value: {  }
default_value_function: ''
settings:
  text_processing: '1'
  translation_sync: false
field_type: text_long

field.field.comment.comment_body.yml

uuid: f86f0984-81c3-4120-af01-1e2b8d64cbb0
langcode: en
status: true
dependencies:
  module:
    - comment
    - text
id: comment.comment_body
name: comment_body
entity_type: comment
type: text_long
settings:
  translation_sync: ''
module: text
locked: false
cardinality: 1
translatable: true
indexes: {  }

does have translatable true...

[edit...]

field.instance.node.article.comment.yml

uuid: 00e655c0-ea28-46d8-b508-8f741b7001a9
langcode: en
status: true
dependencies:
  entity:
    - field.field.node.comment
    - node.type.article
id: node.article.comment
label: 'Comment settings'
field_uuid: ed263fa2-cd72-44ed-9b00-b7dfe6ef43e7
field_name: comment
entity_type: node
bundle: article
description: 'The text I want.'
required: true
translatable: true
default_value:
  -
    status: 2
    cid: 0
    last_comment_timestamp: 0
    last_comment_name: null
    last_comment_uid: 0
    comment_count: 0
default_value_function: ''
settings:
  comment:
    default_mode: 1
    per_page: '50'
    anonymous: 0
    subject: 1
    form_location: 1
    preview: '1'
  default_mode: true
  per_page: 50
  form_location: true
  anonymous: 0
  subject: true
  preview: 1
  translation_sync: false
field_type: comment

field.field.node.comment.yml

uuid: ed263fa2-cd72-44ed-9b00-b7dfe6ef43e7
langcode: en
status: true
dependencies:
  module:
    - comment
    - node
id: node.comment
name: comment
entity_type: node
type: comment
settings:
  comment_type: comment
  translation_sync: ''
module: comment
locked: false
cardinality: 1
translatable: true
indexes: {  }

[edit again]
why, though, is
field.instance.node.article.field_image.yml
translatable: true

and field.field.node.field_image.yml
also
translatable: true
?

maybe... @plach is saying that in
./core/profiles/standard/config/install/field.instance.node.article.field_image.yml
it should have a
translatable: false
?

(right now, there is no translatable: ... in there)

So, /admin/config/regional/content-language
is about content translation, not config translation.

So maybe comment settings should *not* be listed there. (and should not have been *before*), and should be translatable false.

If people want to say that the comments on articles are translatable, then they would use
the comment element to Custom language settings:

For image fields, we dont have title or alt enabled or disabled per language (in core) so we probably shouldn't for comment settings (for content translation).
Config translation would/should probably handle any parts of the comment settings that should be translatable, like the label or the help text.

super! the test actually fails where we want it to. :)

Talked to @fran seva in irc, and they are going to post an updated test with a rename, and few nit fixes, the interdiff for those changes, and also a combined fix and test patch. :)

I went back and looked at the site config export. I think it is being saved ok, (with the patch) with translatable: true in the right places. (given my new understanding of field.field.... )

Also, I did the manual testing:
the comment setting (open, closed, hidden) *can* be set per language for a node. (except for #2294159: Translations of nodes with comment settings field set to translatable are missing hidden choice). disabling translation on the comment settings, and then re-enabling it: each translation remembers its old setting. which was nice.

Taking off the needs test tag, as we had the 3 fails without the fix, and with the fix and test, we have all passes. So I think that is ok.

Updated issue summary

Next here: a (final-ish) review.

issue summary html fix