Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Commit message:

Issue #1987668 by damiankloip, jibran, h3rj4n, mparker17: Convert config_test() module to routes.
Files: 
CommentFileSizeAuthor
#25 1987668-25.patch12.68 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 56,066 pass(es).
[ View ]
#25 interdiff.txt1.07 KBjibran
#21 1987668-21.patch12.29 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 55,684 pass(es).
[ View ]
#21 interdiff.txt2.11 KBjibran
#20 1987668-20.patch14.39 KBjibran
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#20 interdiff.txt4.84 KBjibran
#17 1987668-17.patch10.67 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,511 pass(es).
[ View ]
#17 interdiff-1987668-17.txt2.33 KBdamiankloip
#14 1987668-14.patch10.45 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987668-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 interdiff-1987668-14.txt1.58 KBdamiankloip
#11 1987668-11.patch10.51 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 55,483 pass(es), 63 fail(s), and 35 exception(s).
[ View ]
#11 interdiff-1987668-11.txt4.25 KBdamiankloip
#8 1987668-8.patch10.93 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,435 pass(es).
[ View ]
#8 interdiff-1987668-8.txt1.02 KBdamiankloip
#7 1987668-7.patch11.95 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#7 interdiff-1987668-7.txt3.25 KBdamiankloip
#3 1987668-3.patch12.46 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,796 pass(es).
[ View ]
#2 d8.config_test-routes.patch12.82 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 55,560 pass(es).
[ View ]

Comments

Assigned:Unassigned» damiankloip

Doing this now

Title:Convert config_test_entity_enable() to a new style controllerConvert config_test module to routes
Assigned:damiankloip» Unassigned
Status:Active» Needs review
StatusFileSize
new12.82 KB
PASSED: [[SimpleTest]]: [MySQL] 55,560 pass(es).
[ View ]

Seems easier to do this module in one issue...

Here is a first attempt.

StatusFileSize
new12.46 KB
PASSED: [[SimpleTest]]: [MySQL] 55,796 pass(es).
[ View ]

We can use the _entity_form for adding.

Oh you closed those two (with no link? ouch!) but @h3rj4n (mrtime on IRC) has worked pretty hard on that other one...

Oops, totally missed that a patch had been posted on the add_page issue, sorry @h3rj4n.

I have updated the issues summary to make sure @h3rj4n also gets commit credit for the conversion.

StatusFileSize
new3.25 KB
new11.95 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I spoke to Tim about this, Let's add the default local task back in. There is another issues to fight that one out.

StatusFileSize
new1.02 KB
new10.93 KB
PASSED: [[SimpleTest]]: [MySQL] 55,435 pass(es).
[ View ]

Missed a couple of assertions that needed to be reverted.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestController.phpundefined
@@ -0,0 +1,84 @@
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Drupal\Core\ControllerInterface;
+use Drupal\config_test\Plugin\Core\Entity\ConfigTest;
+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Drupal\Core\Entity\EntityManager;

Just to be consistent ... these should be ordered properly.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestController.phpundefined
@@ -0,0 +1,84 @@
+    return new static($container->get('plugin.manager.entity'));

Maybe i'm stupid, but do you actually use the entity manager at all?

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestController.phpundefined
@@ -0,0 +1,84 @@
+   * Page callback: Presents the ConfigTest edit form.

I don't think we should still refer to page callbacks.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestController.phpundefined
@@ -0,0 +1,84 @@
+   * @param Drupal\config_test\Plugin\Core\Entity\ConfigTest $config_test
...
+   * @param Drupal\config_test\ConfigTest $config_test
...
+   * @param Drupal\config_test\ConfigTest $config_test

Missing "\"

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestController.phpundefined
@@ -0,0 +1,84 @@
+    drupal_set_title(format_string('Edit %label', array('%label' => $config_test->label())), PASS_THROUGH);
+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Form/DeleteForm.phpundefined
@@ -0,0 +1,68 @@
+    drupal_set_message(format_string('%label configuration has been deleted.', array('%label' => $this->configTest->label())));

Use String::format instead.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestController.phpundefined
@@ -0,0 +1,84 @@
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse.
...
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse.

... needs some comment.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Form/DeleteForm.phpundefined
@@ -0,0 +1,68 @@
+class DeleteForm extends ConfirmFormBase {

Needs doc.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Form/DeleteForm.phpundefined
@@ -0,0 +1,68 @@
+  public function buildForm(array $form, array &$form_state, ConfigTest $config_test = NULL) {
+    $this->configTest = $config_test;

Is there a reason why we set a default value? According to submitForm() we expect it to be valid.

StatusFileSize
new4.25 KB
new10.51 KB
FAILED: [[SimpleTest]]: [MySQL] 55,483 pass(es), 63 fail(s), and 35 exception(s).
[ View ]

Thanks dawehner!

Status:Needs review» Needs work

The last submitted patch, 1987668-11.patch, failed testing.

I guess that's another issue with the missing /edit.

Status:Needs work» Needs review
StatusFileSize
new1.58 KB
new10.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987668-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

No, I think the main issue is that I was still implementing ControllerInterface, but removed the create method as we don;t need it anymore :)

Is there a reason why we set a default value? According to submitForm() we expect it to be valid.

Alas, if we don't specify a default value we cannot add an additional parameter, as we are extending buildForm(array $form, array &$form_state). So that has to be back in the patch.

Status:Needs review» Needs work

The last submitted patch, 1987668-14.patch, failed testing.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestController.phpundefined
@@ -0,0 +1,62 @@
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Drupal\config_test\Plugin\Core\Entity\ConfigTest;
+use Drupal\Component\Utility\String;

Most of the time we specify the symfony uses after the drupal ones.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/Form/DeleteForm.phpundefined
@@ -0,0 +1,71 @@
+  public function buildForm(array $form, array &$form_state, ConfigTest $config_test = NULL) {

We should document the new parameter $config_test in here.

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

Still uses format_string

Status:Needs work» Needs review
StatusFileSize
new2.33 KB
new10.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,511 pass(es).
[ View ]

Thanks Daniel.

I am marking #1945444: Convert confirm_form() in config_test.module to the new form interface and convert route as duplicate and I think only this is remaing

+++ 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 from #1945444-26: Convert confirm_form() in config_test.module to the new form interface and convert route point 1.

StatusFileSize
new4.84 KB
new14.39 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
  1. Rename \Drupal\config_test\Form\DeleteForm to \Drupal\config_test\Form\ConfigTestDeleteForm
  2. Added ConfigTestInterface.

StatusFileSize
new2.11 KB
new12.29 KB
PASSED: [[SimpleTest]]: [MySQL] 55,684 pass(es).
[ View ]

Forgot to remove \Drupal\config_test\Form\DeleteForm in last patch.

#21: 1987668-21.patch queued for re-testing.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

updated commit message

Added a commit message to the issue summary

StatusFileSize
new1.07 KB
new12.68 KB
PASSED: [[SimpleTest]]: [MySQL] 56,066 pass(es).
[ View ]

Implemented hook_local_actions().

Status:Needs review» Reviewed & tested by the community

Looks great all in all.

Status:Reviewed & tested by the community» Needs work

we've already used the drupal_set_title() in edit() we could do the same in the other controller methods and the buildForm method of the confirmForm and then hook_menu is history... :)

Status:Needs work» Reviewed & tested by the community

Ignore me... let's wait till we have a proper solution for page titling.

Status:Reviewed & tested by the community» Fixed

Committed 8355142 and pushed to 8.x. Thanks!

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

Issue summary:View changes

Updated issue summary.