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

damiankloip’s picture

Assigned: Unassigned » damiankloip

Doing this now

damiankloip’s picture

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

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

Here is a first attempt.

damiankloip’s picture

FileSize
12.46 KB
PASSED: [[SimpleTest]]: [MySQL] 55,796 pass(es). View

We can use the _entity_form for adding.

tim.plunkett’s picture

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

damiankloip’s picture

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.

damiankloip’s picture

FileSize
3.25 KB
11.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.

damiankloip’s picture

FileSize
1.02 KB
10.93 KB
PASSED: [[SimpleTest]]: [MySQL] 55,435 pass(es). View

Missed a couple of assertions that needed to be reverted.

tim.plunkett’s picture

dawehner’s picture

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

damiankloip’s picture

FileSize
4.25 KB
10.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.

dawehner’s picture

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
10.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.

dawehner’s picture

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
10.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,511 pass(es). View

Thanks Daniel.

jibran’s picture

jibran’s picture

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.

jibran’s picture

FileSize
4.84 KB
14.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.
jibran’s picture

FileSize
2.11 KB
12.29 KB
PASSED: [[SimpleTest]]: [MySQL] 55,684 pass(es). View

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

kim.pepper’s picture

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

jibran’s picture

jibran’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Issue summary: View changes

updated commit message

tim.plunkett’s picture

Added a commit message to the issue summary

jibran’s picture

FileSize
1.07 KB
12.68 KB
PASSED: [[SimpleTest]]: [MySQL] 56,066 pass(es). View

Implemented hook_local_actions().

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks great all in all.

alexpott’s picture

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... :)

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

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.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.