Files: 
CommentFileSizeAuthor
#30 interdiff.txt935 bytesjibran
#30 config_test-1945444-30.patch6.05 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).
[ View ]
#27 interdiff.txt3.52 KBjibran
#27 config_test-1945444-27.patch6.05 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.php.
[ View ]
#25 config_test-1945444-25.patch4.56 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 55,937 pass(es).
[ View ]
#25 interdiff.txt1.82 KBjibran
#16 config_test-1945444-16.patch4.7 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 55,402 pass(es).
[ View ]
#16 interdiff-config_test-16.txt1.21 KBxjm
#13 config_test-1945444-13.patch4.74 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 53,222 pass(es), 18 fail(s), and 11 exception(s).
[ View ]
#13 interdiff-config_test-13.txt1.38 KBxjm
#11 config_test-1945444-11.patch3.63 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#7 config_test-1945444-7.patch3.93 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#7 interdiff-config_test-7.txt1.73 KBxjm
#3 config_test-1945444.patch3.52 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Comments

xjm’s picture

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

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

Status:Active» Needs review
StatusFileSize
new3.52 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

xjm’s picture

Status:Needs review» Needs work
xjm’s picture

+++ 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

+++ 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

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
StatusFileSize
new1.73 KB
new3.93 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
xjm’s picture

StatusFileSize
new3.54 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new397 bytes
xjm’s picture

StatusFileSize
new29.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

StatusFileSize
new774 bytes
new3.63 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

This should fix that.

xjm’s picture

StatusFileSize
new1.38 KB
new4.74 KB
FAILED: [[SimpleTest]]: [MySQL] 53,222 pass(es), 18 fail(s), and 11 exception(s).
[ View ]

Die cruft die.

Status:Needs review» Needs work

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

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
new4.7 KB
PASSED: [[SimpleTest]]: [MySQL] 55,402 pass(es).
[ View ]

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

+++ 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

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

xjm’s picture

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

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

xjm’s picture

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

ParisLiakos’s picture

all this needs now is {inheritdocs} :)

xjm’s picture

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

dawehner’s picture

Status:Needs review» Needs work

So to summarize.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new1.82 KB
new4.56 KB
PASSED: [[SimpleTest]]: [MySQL] 55,937 pass(es).
[ View ]

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

tim.plunkett’s picture

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

Status:Needs work» Needs review
StatusFileSize
new6.05 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestDeleteForm.php.
[ View ]
new3.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

+++ 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

Status:Needs work» Needs review
StatusFileSize
new6.05 KB
PASSED: [[SimpleTest]]: [MySQL] 55,649 pass(es).
[ View ]
new935 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

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

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

jibran’s picture

Status:Needs review» Closed (duplicate)