Support from Acquia helps fund testing for Drupal Acquia logo

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

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

Here is a first attempt.

damiankloip’s picture

FileSize
12.46 KB

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

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

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

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

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

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

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

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.