- ๐ฉ๐ชGermany sascha_meissner Planet earth
#68 was failing to apply on 9.5.3 so i rerolled it to work with 9.5.3
- ๐ฉ๐ชGermany sascha_meissner Planet earth
Luckily it passes for 10.1.x as well ;-)
- ๐บ๐ธUnited States smustgrave
Since this about required field think we need a follow up for #70.5 then this may be ready for review
and @sascha_meissner please include interdiffs with your patches even if they're just rerolls too.
- ๐ฉ๐ชGermany sascha_meissner Planet earth
Sorry, i focused more on the reroll than on the actual issue.
So i spent some hours into re-evealute the issue in 10.x / 9.5.3
To keep the vanilla-state short: The initial errors might have gone but still you can create odd scenarious mentioned above because the form is not dynamic.
To really mitigate all of those symptoms the form needs two "simple" steps in updating itself before you submit it.
1. Insert/Enable
require summary
only ifSummary input
is checked
2. (the missing step) Ifrequire summary
is checked theSummary Default value input
must be inserted/enabled without having to submit the form first.So i tried to code a solution but am really struggling to utilize ajaxcommands to rebuild the whole form, especially the default_value_widget from the element context..
- Status changed to Needs review
over 1 year ago 7:47pm 7 April 2023 - ๐บ๐ธUnited States smustgrave
Still convinced 70.5 can be a separate issue. Having the summary appear when checked could be a nice novice follow up.
This got me again today where I needed to edit a setting of a required field and couldn't get past the field.
Fixed up the patch with latest dev branch.
- Status changed to Needs work
over 1 year ago 11:00pm 10 April 2023 - ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
#70.5 is a false positive, the 'edit summary' link is shown in the screenshot, that is the expected behaviour. The field is only shown expanded by default if 'require summary' is checked.
-
+++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWithSummaryWidget.php @@ -73,7 +73,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen + $required = $display_summary && !$form_state->has('default_value_widget') && $this->getFieldSetting('required_summary'); @@ -88,7 +88,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen + if (!$this->getSetting('show_summary') && !$required && !str_starts_with($route_name, 'entity.field_config.')) {
doesn't the $form_state->has('default_value_widget') make the route name check redundant?
this also prevents other modules (e.g. field_union) from reusing this form
-
+++ b/core/modules/text/tests/src/Kernel/TextSummaryTest.php @@ -251,8 +253,49 @@ public function assertTextSummary(string $text, string $expected, ?string $forma + public function testRequiredSummary($display_summary, $require_summary, $expected_entity_form, $expected_field_config_form) {
In my last review I asked for type hints here, still needs to be actioned
-
- Status changed to Needs review
over 1 year ago 9:27pm 12 April 2023 - ๐บ๐ธUnited States smustgrave
Addressed the points in #80
this also prevents other modules (e.g. field_union) from reusing this form
Not sure I follow or know how to fix that? But if they're using this form wouldn't the default_value_widget check we are doing still apply?
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
I think using default_value_widget is fine - other modules using the widget need to pass that too. The issue I have is with checking the route name
- Status changed to Needs work
over 1 year ago 3:27am 8 May 2023 - ๐ฉ๐ชGermany FeyP
This issue is being reviewed by the kind folks in Slack โ , #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request โ as a guide.
- The issue summary refers to "Outstanding questions", which is also listed as point 3. in the remaining tasks section. I don't see that these questions have been answered and even then, this should be completed/removed. Also, the URLs for the block UI in the steps to reproduce changed in 10.1 and should be updated. Tagging for IS update.
-
#70.5 is a false positive, the 'edit summary' link is shown in the screenshot, that is the expected behaviour. The field is only shown expanded by default if 'require summary' is checked.
- I think #70.5 and #77.2 refer to the same thing, i.e. not being able to set a default value for the summary directly after checking the "Enable summary" checkbox. I agree that this could be a follow-up issue, if needed. Not tagging, because I'm not sure that it's really such an issue that you have to save the field first before you can enter a default value.
- Reviewing the code in #82 I have only one nit: The data sets in the data provider could be named (e.g. have keys) to make it more clear what each data set is testing and allow for easier filtering without having to "count" the data sets. This isn't a requirement though and with the most important two values of the data set right at the top it is not such a big problem, so I wouldn't requite that for RTBC.
- I tested the patch both with the body field for articles and for blocks. Unfortunately, I think there is something wrong still.
- Go to
/admin/structure/block-content/manage/basic/fields/block_content.basic.body
- Check "Summary input" and "Require summary"
- Click "Save settings"
- Go to
/admin/structure/block-content/manage/basic/fields/block_content.basic.body
again - Uncheck "Summary input"
- Click "Save settings"
- Verify that the field is saved successfully with message "Saved Body configuration."
- Go to
/admin/structure/block-content/manage/basic/fields/block_content.basic.body
again. - Verify that both "Summary input" and "Require summary" are unchecked.
In step 7 I would have expected that the field validation would fail and I get an error message that I need to uncheck "Require summary" before I can uncheck "Summary input", not that "Require summary" is silently removed. Are we sure that this is really the expected behavior?
- Go to
Moving to Needs work for the IS update and my testing results, since I'm not sure that this is the desired behavior.
- Merge request !5004Issue #3115978: After enabling Require Summary on a field I canโt create a new entity or disable Require Summary. โ (Open) created by smustgrave
- Open on Drupal.org โEnvironment: PHP 8.2 & MySQL 8last update
about 1 year ago Not currently mergeable. - last update
about 1 year ago 30,387 pass, 2 fail - last update
about 1 year ago 30,400 pass - Status changed to Needs review
about 1 year ago 5:04pm 13 October 2023 - ๐บ๐ธUnited States smustgrave
Hiding the patches as switching to an MR.
Cleaned out some crust from the issue summary to make it easier to read. Addressed the outstanding questions. Think 2/3 are follow ups. Tried to make it more clear too, but how I read this issue, was that there is a bug where you can't edit a text field after making the summary required.
- ๐ฉ๐ชGermany rkoller Nรผrnberg, Germany
I've manually tested the latest patch in #86 ๐ After enabling Require Summary on a field can't save the field Needs review on a fresh install of drupal 11.x-dev. functionally everything behaves as expected and i haven't noticed anything striking. looks good!
the only detail i wonder about is why to grey out the
require summary
checkbox as long as thesummary input
checkbox is unchecked. disabled interface components are always sort of confusing and make me as a user think for a second why the checkbox is greyed out and how i am able to reenable it. and from an accessibility perspective the disabled checkboxes bordercolor (#bababf) has a color contrast of 1.9:1 against the background color white which isn't meeting WCAG SC 1.4.11 which requires at least a contrast of 3:1. Why not apply progressive disclosure and instead of greying out the checkbox hide the checkbox and it's label as long as thesummary input
checkbox is unchecked?overall i would simply trying to avoid using a disabled interface components pattern. might be suitable for a follow up issue or should it be already handled within this issue?
- last update
about 1 year ago 30,400 pass - ๐บ๐ธUnited States smustgrave
Updated state to hide required field when summary unchecked.
- Status changed to Needs work
about 1 year ago 6:31pm 13 October 2023 - ๐ฉ๐ชGermany rkoller Nรผrnberg, Germany
thank you for the fast update! i've quickly applied the updated patch.
1. that is odd, try the following:
- summary input checked & require summary unchecked.
- now uncheck summary input
- now recheck summary input=> summary input and require summary are both checked now. i would have expected that only summary input is checked.
2. just check require summary and save the settings (tried it on a block type's body field)
=> when going into the field settings again after saving both summary input and require summary are both checked in contrast to the previous saved settings?
- ๐บ๐ธUnited States smustgrave
So this may be a bug with the states API. I left the state for unchecking but seems doesn't matter when it's hidden. So think we should revert back to disabled vs hiding.
- Status changed to Needs review
about 1 year ago 6:47pm 13 October 2023 - ๐บ๐ธUnited States smustgrave
Actually may be a problem with the unchecked state.
'unchecked' => [ ':input[name="settings[display_summary]"]' => ['checked' => FALSE], ],
Seems it does the opposite when that condition isn't met.
Unless there is something I'm missing the current setup may be the best we can get.
- Status changed to Needs work
about 1 year ago 1:17am 16 October 2023 - ๐ฉ๐ชGermany FeyP
I think #87 is covered by ๐ Claro: Form labels that are disabled have too low color contrast Needs work , which mentions border color of disabled elements in the IS, so we don't need a follow-up for that.
Form API states has various issues already filed against it, but doing a quick search, I didn't find any obvious candidates for #92. The tests only cover usage with TRUE, not with FALSE. I think we should file an issue for this, if it doesn't exist already, but I don't think we need to postpone on this. But I also think we shouldn't commit it like this, so let's go back to the previous behavior. We could then file another follow-up to use the current behavior using Form API states in this case to improve usability and post-pone that PP2 on this issue and the new follow-up for Form API states.
Tagging Needs Followup again for that.
- last update
about 1 year ago 30,437 pass - Status changed to Needs review
about 1 year ago 4:55pm 24 October 2023 - ๐บ๐ธUnited States smustgrave
Opened up ๐ States API visible stops others states Active and reverted states to us enabled/unchecked
- Status changed to RTBC
about 1 year ago 7:47pm 7 November 2023 - ๐ง๐ชBelgium BramDriesen Belgium ๐ง๐ช
Was looking in dept at the code, had a few ๐ค moments when looking at the tests and added comments, but figured it out when looking at the files as a whole and not only the code changes, so I then deleted my comments ๐ .
Added one nit but that should not stop if from getting RTBC I think.
- last update
about 1 year ago 30,496 pass - last update
about 1 year ago 30,515 pass - last update
about 1 year ago 30,522 pass - last update
about 1 year ago 30,533 pass - last update
about 1 year ago 30,557 pass - last update
about 1 year ago 30,605 pass - last update
about 1 year ago 30,607 pass - last update
about 1 year ago 30,608 pass - last update
about 1 year ago 30,670 pass - last update
about 1 year ago 30,678 pass - last update
about 1 year ago 30,682 pass - last update
about 1 year ago 30,687 pass - Status changed to Needs work
about 1 year ago 5:50am 2 December 2023 The Needs Review Queue Bot โ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide โ to find step-by-step guides for working with issues.
- Status changed to RTBC
about 1 year ago 3:12pm 2 December 2023 - last update
about 1 year ago 30,691 pass - last update
about 1 year ago 30,691 pass - last update
about 1 year ago 30,699 pass - last update
about 1 year ago 30,701 pass - last update
about 1 year ago 30,701 pass - last update
about 1 year ago 30,715 pass - last update
about 1 year ago 30,729 pass - last update
about 1 year ago 30,769 pass - last update
about 1 year ago 25,904 pass, 1,813 fail - last update
about 1 year ago 25,853 pass, 1,835 fail - ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
Updating issue credits
- ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
larowlan โ changed the visibility of the branch 3115978-after-enabling-require to hidden.
- Status changed to Needs work
about 1 year ago 5:33am 22 December 2023 - ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
As someone who worked on the original issue that introduced this bug at least 10 years ago - I really wanted to commit this.
But learning the mistakes from the past, I thought I would give it one more manual test.
I installed demo umami and checked the configuration of the body field on the article node type.
$ drush cget field.field.node.article.body uuid: 5c641576-be7f-4129-b953-b314defaa178 langcode: en status: true dependencies: config: - field.storage.node.body - node.type.article module: - text _core: default_config_hash: 4Wxs1LseZ8JsNwD9uJVIbzZqG35yQXN8Eo1Hub6NMuI id: node.article.body field_name: body entity_type: node bundle: article label: Body description: '' required: false translatable: true default_value: { } default_value_callback: '' settings: display_summary: true required_summary: false allowed_formats: { } field_type: text_with_summary
As you can see there
required:summary: false
BUT if I visit
/en/admin/structure/types/manage/article/fields/node.article.body
without making any changes, the checkbox is tickedI suspected this might be an issue with the
#states
added to that form, so I disabled Javascript and reloaded the page.And sure enough, the checkbox showed the correct default value.
So unfortunately, this is still needs work folks ๐ญ๐ญ๐ญ๐ญ
Adding 'needs manual testing' for the next MR
- Status changed to Needs review
12 months ago 1:10pm 2 January 2024 - ๐บ๐ธUnited States capysara
I added a new condition, based on an example in 2477053 โ where the Require Summary field is set to unchecked if it's not checked. It feels a bit dirty, but I think a better fix could be out of scope for this issue.
I re-added the `visible` condition, see #88 ๐ After enabling Require Summary on a field can't save the field Needs review .
I kept the `enabled` condition because otherwise it attempts to validate Require summary instead of just unchecking it when Summary Input is unchecked and then the form is saved without the user explicitly unchecking Require summary also.I manually tested by doing lots of checking and unchecking the fields, and it behaved as I expected.
- Status changed to Needs work
12 months ago 1:13pm 2 January 2024 - Status changed to Needs review
12 months ago 1:42pm 2 January 2024 - ๐บ๐ธUnited States capysara
Updated the
unchecked
condition to use the defaultOR
statement. - Status changed to RTBC
12 months ago 2:53pm 2 January 2024 - ๐บ๐ธUnited States smustgrave
Tested new changes by creating a text formatted field.
Before when I added a default and made summary required. When I went to edit the field I couldn't save.
That issue appears to be addressed still. - last update
12 months ago 30,779 pass, 13 fail - last update
12 months ago 25,927 pass, 1,832 fail - last update
12 months ago 25,968 pass, 1,867 fail - Status changed to Needs work
12 months ago 9:16pm 7 January 2024 - ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
The same issue still exists.
Steps to reproduce
* Install Umami
* Inspect node article field withdrush cget field.field.node.article.body
and take note that summary isn't required
* Visit /en/admin/structure/types/manage/article/fields/node.article.body and see the checkbox is checked when it shouldn't be - Status changed to Needs review
12 months ago 3:16pm 8 January 2024 - ๐บ๐ธUnited States smustgrave
Removed 1 of the checks. As discussed in the 80s-90s comments appears there may be a bug in states API, followups were opened
- Status changed to RTBC
12 months ago 3:49pm 10 January 2024 - ๐ง๐ชBelgium BramDriesen Belgium ๐ง๐ช
Code wise this also looks good! Previous remarks are implemented and all quality gates are green.
- Status changed to Needs work
12 months ago 9:50am 11 January 2024 - ๐ฌ๐งUnited Kingdom alexpott ๐ช๐บ๐
Added some review questions to the MR. Also what is interesting and we should work out why - node fields are not affected by the same bug. If you set them to have a required summary but do not display the summary nothing in broken. I suspect that with this change there is a workaround we can remove.
- ๐ฎ๐นItaly kopeboy Milan
Please also note that currently the Summary is required for as many default values as the field storage cardinality!
- ๐บ๐ธUnited States capysara
Let's keep this on track for 11 and not add patches when there's already work in MR.
Hiding the patch to avoid confusion. - ๐ณ๐ฟNew Zealand quietone
Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.