Follow-up of #1892182: #type details: Rename #collapsed to #open (comments #58 and #59) where #collapsible and #collapsed for detail elements is replaced by #open and the default is reversed. Following that idea, some logic and variable naming should be inversed as well.

This is a 3 line change patch for file/class/method core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase::buildConfigForm(): where parameter $collapsed should be renamed to $open, including its logic default and thus also including changing calls to it that pass or not pass that option.

CommentFileSizeAuthor
#20 interdiff-2203021-9-20.txt2.08 KBjorgegc
#20 drupal-rename-to-open-2203021-20.patch2.56 KBjorgegc
#16 interdiff-2203021-9-16.txt1.44 KBjorgegc
#16 drupal-rename-to-open-2203021-16.patch3.12 KBjorgegc
#9 drupal-rename-to-open-2203021-9.patch2.65 KBmandar.harkare
#5 drupal-rename-to-open-2203021-5.patch1.73 KBmandar.harkare
#2 drupal-rename-to-open-2203021-2.patch1.29 KBmandar.harkare

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase::buildConfigForm(): rename $collapsed to $open » Rename $collapsed argument to $open in ConfigTranslationFormBase::buildConfigForm()
Status: Active » Postponed

Slightly more sane issue title ;)

+ postponing on parent issue.

mandar.harkare’s picture

mandar.harkare commented
StatusFileSize
new drupal-rename-to-open-2203021-2.patch1.29 KB
fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented

Thanks for posting the patch. A few remarks though

+++ core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php	(revision )
@@ -259,7 +259,7 @@
-  protected function buildConfigForm(Element $schema, $config_data, $base_config_data, $collapsed = FALSE, $base_key = '') {
+  protected function buildConfigForm(Element $schema, $config_data, $base_config_data, $open = FALSE, $base_key = '') {
     $build = array();

This issue is slightly more than a simple rename, as we also inverse the meaning of the parameter. So either:
- the default should become TRUE
- the default remains FALSE, as is the default for the open attribute in a details element. So I suggest to use this case.

But then:
- in the former case, the default behavior does not change and we only must change calls to this method that do pass in a value for this parameter.
- in the latter case, the default behavior changes also and thus must we change all calls to this method.

It is a protected method and only 2 sub classes call this method, so it won't be to difficult to find these calls and cahnge those lines as well (change: true => false, false => true, expression => !expression).

+++ core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php	(revision )
@@ -294,7 +294,7 @@
-            '#collapsed' => $collapsed,
+            '#open' => $open,
           ) + $sub_build;

This will need to be rerolled once the parent issue gets in, as the parent issue changes this line to '#open'=> !$collapsed,

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented
Status: Postponed » Needs work

Parent issue is in. Above patch is not yet OK. Setting to Needs Work.

mandar.harkare’s picture

mandar.harkare commented
StatusFileSize
new drupal-rename-to-open-2203021-5.patch1.73 KB

Not sure if the first call needs to add the TRUE parameter, adding it though.

mandar.harkare’s picture

mandar.harkare commented
Status: Needs work » Needs review

Please review it.

The last submitted patch, 2: drupal-rename-to-open-2203021-2.patch, failed testing.

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented
Status: Needs review » Needs work

The contents of the patch from #5 is correct, but needs to be combined with the contents of the patch in #2. Both together will give us a patch that will fix this issue.

mandar.harkare’s picture

mandar.harkare commented
Status: Needs work » Needs review
StatusFileSize
new drupal-rename-to-open-2203021-9.patch2.65 KB
fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented
Status: Needs review » Reviewed & tested by the community

This is how it should be. Good to go.

tstoeckler’s picture

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

Thanks for working on this patch, looks great but for one minor detail.

This is fairly minor, but sending back to "needs review" to see what people think:

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
@@ -188,7 +188,7 @@ public function buildForm(array $form, array &$form_state, Request $request = NU
-      $form['config_names'][$name] += $this->buildConfigForm($this->typedConfigManager->get($name), $this->config($name)->get(), $this->baseConfigData[$name]);
+      $form['config_names'][$name] += $this->buildConfigForm($this->typedConfigManager->get($name), $this->config($name)->get(), $this->baseConfigData[$name], TRUE);

@@ -259,7 +259,7 @@ public function submitForm(array &$form, array &$form_state) {
-  protected function buildConfigForm(Element $schema, $config_data, $base_config_data, $collapsed = FALSE, $base_key = '') {
+  protected function buildConfigForm(Element $schema, $config_data, $base_config_data, $open = FALSE, $base_key = '') {

Shouldn't we change the default value to "TRUE" instead?

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented

- The default for a details element is to be closed, so that wold suggest $open = false.
- OTOH, this (protected) function is called twice, once outside this function and once recursively inside this function. With this new default, both calls now have to pass in a value, so the default value for this parameter can even be removed. It even looks like the last 2 parameters were for "internal use" only. So that would suggest $open= true.

I have no clear opinion on this, so I will RTBC both ways.

@mandar.harkare: if you can post another patch with $open= TRUE, anyone with a clear preference can choose :)

lhangea’s picture

lhangea commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I have a clear preference for reversing the default since we've reversed all the other logic.

jorgegc’s picture

jorgegc commented

I am going to work on a patch to address #14.

jorgegc’s picture

jorgegc commented
Status: Needs work » Needs review
StatusFileSize
new drupal-rename-to-open-2203021-16.patch3.12 KB
new interdiff-2203021-9-16.txt1.44 KB

This was my first core patch, I think the next step is getting a review and seeing what the testbot results are.

I've basically reversed the default and updated the PHPDoc accordingly.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work 
+++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
@@ -188,7 +188,7 @@ public function buildForm(array $form, array &$form_state, Request $request = NU
-      $form['config_names'][$name] += $this->buildConfigForm($this->typedConfigManager->get($name), $this->config($name)->get(), $this->baseConfigData[$name]);
+      $form['config_names'][$name] += $this->buildConfigForm($this->typedConfigManager->get($name), $this->config($name)->get(), $this->baseConfigData[$name], TRUE);

This change can now be reverted since TRUE is the default

tstoeckler’s picture

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

@jorgegc: Wow, nice job for your first patch!!! Maybe you can fix these minor issues and then we can set this to RTBC?!

  1. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Form/ConfigTranslationFormBase.php
@@ -251,15 +251,15 @@ public function submitForm(array &$form, array &$form_state) {
-   * @param bool $collapsed
-   *   (optional) Flag to set collapsed state. Defaults to FALSE.
+   * @param bool $open
+   *   (optional) Flag to set collapsed state. Defaults to TRUE.

    "Flag to set collapsed state" is sort of not wrong, but "Defaults to TRUE" implies that it defaults to collapsed, while it defaults to open. Maybe something like:
    "Whether or not the details element of the form should be open. Defaults to TRUE."

Edit: Fix broken markup.

jorgegc’s picture

jorgegc commented

The work was done by @mandar.harkare, I am just making some small changes :-) Thanks for the feedback guys, I will redo my patch and interdiff.

jorgegc’s picture

jorgegc commented
StatusFileSize
new drupal-rename-to-open-2203021-20.patch2.56 KB
new interdiff-2203021-9-20.txt2.08 KB

Just uploaded a patch and an interdiff to implement changes requested in #17 and #18.

jorgegc’s picture

jorgegc commented
Status: Needs work » Needs review

Could you guys please review?

fietserwin’s picture

fietserwin
Location Le Mont-Dore
commented
Status: Needs review » Reviewed & tested by the community

#18: good catch.

RTBC, under the assumption that the testbot can apply it and all tests still succeed.

tstoeckler’s picture

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

Awesome, thanks for the quick turnaround. RTBC++

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed f658a90 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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