Part of #1945406: [meta] Convert all of confirm_form() to ConfirmFormBase.

CommentFileSizeAuthor
#30 interdiff.txt935 bytesjibran
#30 config_test-1945444-30.patch6.05 KBjibran
#27 interdiff.txt3.52 KBjibran
#27 config_test-1945444-27.patch6.05 KBjibran
#25 config_test-1945444-25.patch4.56 KBjibran
#25 interdiff.txt1.82 KBjibran
#16 config_test-1945444-16.patch4.7 KBxjm
#16 interdiff-config_test-16.txt1.21 KBxjm
#13 config_test-1945444-13.patch4.74 KBxjm
#13 interdiff-config_test-13.txt1.38 KBxjm
#11 config_test-1945444-11.patch3.63 KBxjm
#11 interdiff-config_test-11.txt774 bytesxjm
#10 almost.png29.63 KBxjm
#9 interdiff-config_test-8.txt397 bytesxjm
#9 config_test-1945444-8.patch3.54 KBxjm
#7 config_test-1945444-7.patch3.93 KBxjm
#7 interdiff-config_test-7.txt1.73 KBxjm
#3 config_test-1945444.patch3.52 KBxjm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Convert confirm_form() in config.module to the new form interface » Convert confirm_form() in config.module to the new form interface and convert routes
Issue tags: +FormInterface, +WSCCI-conversion
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Convert confirm_form() in config.module to the new form interface and convert routes » Convert confirm_form() in config_test.module to the new form interface and convert routes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Active » Needs review
FileSize
config_test-1945444.patch3.52 KB

First part of the conversion. The form doesn't have any buttons, but other than that, it works great!

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Needs work
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.phpundefined
@@ -0,0 +1,75 @@
+  /**
+   * The ConfigTest object to delete.
+   *
+   * @var \Drupal\config_test\Plugin\Core\Entity\ConfigTest
+   */
+  protected $id;

Oops.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.phpundefined
@@ -0,0 +1,75 @@
+    parent::buildForm($form, $form_state);

Oh hey, maybe I should actually return the form! ...Nah. Crazy talk.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Title: Convert confirm_form() in config_test.module to the new form interface and convert routes » Convert confirm_form() in config_test.module to the new form interface and convert route
Status: Needs work » Needs review
FileSize
interdiff-config_test-7.txt1.73 KB
config_test-1945444-7.patch3.93 KB
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
config_test-1945444-8.patch3.54 KB
interdiff-config_test-8.txt397 bytes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
almost.png29.63 KB

Works fine now in the browser, aside from that it doesn't redirect to a sensible path after deleting the item:

almost.png

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
interdiff-config_test-11.txt774 bytes
config_test-1945444-11.patch3.63 KB

This should fix that.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
FileSize
interdiff-config_test-13.txt1.38 KB
config_test-1945444-13.patch4.74 KB

Die cruft die.

Status: Needs review » Needs work

The last submitted patch, config_test-1945444-13.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Needs review
FileSize
interdiff-config_test-16.txt1.21 KB
config_test-1945444-16.patch4.7 KB

Fixed the fails due to out-of-scope changes (yes, I know better) and over-enthusiastic use of new toys. The last fail is legit, due to #1915752: Routes are not found when 0 is used as a placeholder value..

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented 
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.phpundefined
@@ -0,0 +1,76 @@
+   * @param \Drupal\config_test\Plugin\Core\Entity\ConfigTest $config_test
+   *   (optional) The ConfigTest object to delete. If NULL, your form is
+   *   probably broken, but the interface gets pissy otherwise.

@tim.plunkett and I agree this is going to cause problems. Maybe we should file an issue to try to fix it.

Also, note to self, make this @param more, uh, standard in the next reroll. ;)

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.phpundefined
@@ -0,0 +1,76 @@
+    $form_state['redirect'] = 'admin/structure/config_test';

I wonder if there's a nicer way we could add the redirect?

Status: Needs review » Needs work

The last submitted patch, config_test-1945444-16.patch, failed testing.

Crell’s picture

Crell CreditAttribution: Crell commented

I'm curious... why does a test module have a confirm form in the first place? That seems... excessive.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs work » Needs review
Issue tags: -FormInterface, -WSCCI-conversion

#16: config_test-1945444-16.patch queued for re-testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +FormInterface, +WSCCI-conversion

#16: config_test-1945444-16.patch queued for re-testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

all this needs now is {inheritdocs} :)

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented

#16: config_test-1945444-16.patch queued for re-testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Needs work

So to summarize.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.82 KB
config_test-1945444-25.patch4.56 KB

I hope @xjm will not mind that I have rerolled the issues assign to her. Fixed #22.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Needs work 
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.phpundefined
@@ -0,0 +1,76 @@
+use Drupal\config_test\Plugin\Core\Entity\ConfigTest;
...
+   * @var \Drupal\config_test\Plugin\Core\Entity\ConfigTest
...
+   * @param \Drupal\config_test\Plugin\Core\Entity\ConfigTest $config_test
...
+  public function buildForm(array $form, array &$form_state, ConfigTest $config_test = NULL) {

This should typehint with ConfigTestInterface, but it seems I forgot that one in #1391694: Use type-hinting for entity-parameters.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.phpundefined
@@ -0,0 +1,76 @@
+   *   (optional) The ConfigTest object to delete. If NULL, your form is
+   *   probably broken, but the interface gets pissy otherwise.

This still needs a better comment

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.phpundefined
@@ -0,0 +1,76 @@
+    $label = $this->configTest->label();
...
+    drupal_set_message(format_string('%label configuration has been deleted.', array('%label' => $label)));

This should use String::format(), and you don't need the variable for $label, just do it inline

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
FileSize
config_test-1945444-27.patch6.05 KB
interdiff.txt3.52 KB

Thanks @tim.plunkett for the review and help in fixing the issues. Addressed all the issues in #26.

Status: Needs review » Needs work

The last submitted patch, config_test-1945444-27.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.phpundefined
@@ -66,9 +66,8 @@ public function buildForm(array $form, array &$form_state, ConfigTest $config_te
+    drupal_set_message(String::format('%label configuration has been deleted.', array('%label' => $this->configTest->label()));

I think this is missing a closing )

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
FileSize
config_test-1945444-30.patch6.05 KB
interdiff.txt935 bytes

Sorry about that.

Status: Needs review » Needs work
Issue tags: -FormInterface, -WSCCI-conversion

The last submitted patch, config_test-1945444-30.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
Issue tags: +FormInterface, +WSCCI-conversion

#30: config_test-1945444-30.patch queued for re-testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Closed (duplicate)

This is the part of #1987668: Convert config_test module to routes.