Create a Boolean field on the Page content type, with name "field_test".

Then go to admin/structure/types/manage/page/fields/node.page.field_test/storage (which is the "Edit storage" page for that field).

You will see the on/off labels shown on this screen. But this page says "These settings impact the way that data is stored in the database and cannot be changed once data has been created."

Um. I don't think so? How are the on/off labels database storage settings? These need to be moved to the ordinary field settings that you can edit later.

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because you should be able to change labels regardless of whether or not there is content in the database
Issue priority Major because this doen't allow for boolean labels to be changed after content has been added.
Prioritized changes The main goal of this issue is fixing a bug.
CommentFileSizeAuthor
#38 2434745-38-complete.patch8.35 KBgeertvd
#38 interdiff.txt773 bytesgeertvd
#35 2434745-35-complete.patch8.24 KBgeertvd
#35 interdiff.txt1.11 KBgeertvd
#31 2434745-31-complete.patch7.13 KBgeertvd
#31 2434745-31-test.patch1.02 KBgeertvd
#26 2434745-25.patch6.39 KBgeertvd
#21 2434745-21.patch1.38 MBgeertvd
#21 interdiff.txt513 bytesgeertvd
#15 field_settings_boolean.png105.3 KBgeertvd
#11 2434745-11.patch6.36 KBgeertvd
#9 2434745-9.patch6.23 KBgeertvd
#6 2434745-6.patch5.76 KBgeertvd
#3 2434745-3.patch3.87 KBgeertvd
#2 2434745-2.patch1.16 KBgeertvd
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Active » Needs review

Seems weird indeed, fix is pretty easy.
This should probably break some tests.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2434745-2.patch1.16 KB

Woops forgot to add the patch

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2434745-3.patch3.87 KB

This should take care of the breaking tests

The last submitted patch, 2: 2434745-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2434745-3.patch, failed testing.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2434745-6.patch5.76 KB
geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: 2434745-6.patch, failed testing.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2434745-9.patch6.23 KB
geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review
geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2434745-11.patch6.36 KB

The last submitted patch, 9: 2434745-9.patch, failed testing.

geertvd’s picture

geertvd CreditAttribution: geertvd commented

Should we still write extra tests for this? I guess we could create a boolean field, add some content and then try to change the on/off labels or is that unnecessary?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

It looks like you already had to update several tests in your patch, so the functionality of "change on/off text settings" looks like it's been adequately tested.

A screen shot of this working might be good. :)

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
field_settings_boolean.png105.3 KB

Just added a screenshot of the settings page.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Looks good!

- I tried this out on simplytest.me. I added a Boolean field to the Article content type of the standard profile. Checked the Manage Form Display page and Manage Display. Created some content and looked at the form and the output. It is all working fine. @geertvd also added a screen shot.

- Since this patch had to revise tests, I think the functionality has been adequately tested.

- The code in the patch looks good. I grepped for "off_label" in Core (on_label gave me a bunch of button_label hits, oof!) and it looks like the patch is covering most everything... but there were a few missed, which is a concern:

core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/BooleanFormatter.php:    
  'default' => [$this->getFieldSetting('on_label'), $this->getFieldSetting('off_
label')],

And in addition to the two spots that were fixed in core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php there were two other spots that were not touched.

So can you explain why these 3 places in the Core code do not need to also change? I think they might... happy to be wrong but I think they at least need an explanation.

We'll also need to add a Beta Evaluation to the issue summary.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Issue summary: View changes

I think $this->getFieldSetting('on_label') doesn't care whether this is a setting on field storage or on the field instance.
It just points to $this->fieldDefinition->getSetting($setting_name);which just returns any field setting.

The 2 other spots in core/lib/Drupal/Core/Field/Plugin/Field/FieldType/BooleanItem.php, i'm assuming you're talking about 

  return array(
      0 => $this->getSetting('off_label'),
      1 => $this->getSetting('on_label'),
    );

This is the same explanation as above.

I don't know if that answers your question?

added Beta Evaluation

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community

Ah, you're right. Thanks for increasing my knowledge of the Field API. I took a closer look at those 3 spots and I agree with your analysis. I think this is ready to go -- great work!

amateescu’s picture

amateescu CreditAttribution: amateescu commented 
+++ b/core/config/schema/core.data_types.schema.yml
@@ -585,6 +585,10 @@ field.value.entity_reference:
 field.storage_settings.boolean:
   type: mapping
   label: 'Boolean settings'

This part of config schema seems to be unnecessary now because there are no storage settings to save in config.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
interdiff.txt513 bytes
2434745-21.patch1.38 MB

Removed that bit.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Reviewed & tested by the community » Needs review
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

Awesome! Let's hope the testbot agrees :)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented
Status: Reviewed & tested by the community » Needs work

Patch size is a bit big for my taste ;-)

(And it reverts the latest core commits...)

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

Also, since we are already changing this, why don't we move this to a widget setting directly? The boolean formatter recently gained the ability to configure the labels so it's really only relevant for widgets. That would also solve #2428087: Boolean field formatter - default choice appears in list twice more elegantly and also the weird UX fail that of the two labels you currently have to enter in the field settings only one is ever used with the CheckboxWidget and it's not inherently obvious which one.

Thoughts?

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review
FileSize
2434745-25.patch6.39 KB

woops :)

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Assigned: Unassigned » geertvd

#25 Sounds like the right approach, I'll look into to this tomorrow.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs review » Needs work
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

RE #25 - no. It cannot be a widget setting, because it is used in more than just the widget. The formatter also looks at this field setting, because one of its options is to display with the field settings strings. Also there is a method on the field class called getOptions(), or something like that, which returns options that can be used for a Select or Radios widget, and that also needs something to return (which should be settable by the user and therefore needs to be on the field settings).

So. The approach here is correct. Do not move this setting to the widget.

The patch in #25 looks good. No extra pieces. It looks like the patch I approved in #11 with the changes suggested/approved by amateescu.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Nice catch.

Only issue is I don't believe I see any explicit test for the STR in the issue summary? (All the test hunks seem to simply be updating based on the changes this patch introduces.) Would be really nice to not ever break this again.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2434745-31-test.patch1.02 KB
2434745-31-complete.patch7.13 KB

This tests if we can change the label after creating content.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review

The last submitted patch, 31: 2434745-31-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 31: 2434745-31-complete.patch, failed testing.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.11 KB
2434745-35-complete.patch8.24 KB

Complete patch in #31 failed because some new boolean tests were added in BooleanFormatterSettingsTest.php

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Assigned: geertvd » Unassigned

Status: Needs review » Needs work

The last submitted patch, 35: 2434745-35-complete.patch, failed testing.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review
FileSize
interdiff.txt773 bytes
2434745-38-complete.patch8.35 KB
jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

The latest patch looks good to me, and to the testbot. The new test appears to be sufficient to test the new functionality. Thanks again!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +D8 upgrade path

Committed/pushed to 8.0.x, thanks!

Retrospectively tagging 'D8 upgrade path'.

  • catch committed bbc47f0 on 8.0.x 
    Issue #2434745 by geertvd: Boolean field: On/Off labels are in the "...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.