config_test module used to test Configurables but still does not uses EntityFormController

entity_test module could be a good example of this conversion

Files: 
CommentFileSizeAuthor
#38 config.test-form.38.patch28.8 KBsun
PASSED: [[SimpleTest]]: [MySQL] 46,383 pass(es).
[ View ]
#38 interdiff.txt2.38 KBsun
#33 config.test-form.34.patch29.19 KBsun
PASSED: [[SimpleTest]]: [MySQL] 46,321 pass(es).
[ View ]
#33 interdiff.txt3.31 KBsun
#29 config.test-form.29.patch26.15 KBsun
FAILED: [[SimpleTest]]: [MySQL] 46,274 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#24 config.test-form.24.patch26.38 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.test-form.24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#21 config.test-form.21.patch26.36 KBsun
PASSED: [[SimpleTest]]: [MySQL] 42,895 pass(es).
[ View ]
#21 interdiff.txt26.83 KBsun
#19 config_test-1798944-19.patch14.23 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 42,162 pass(es).
[ View ]
#19 interdiff.txt5.32 KBkbasarab
#17 config_test-1798944-17.patch11.03 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] 42,173 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
#17 interdiff.txt1.96 KBkbasarab
#16 config_test-1798944-16.patch10.41 KBkbasarab
PASSED: [[SimpleTest]]: [MySQL] 42,179 pass(es).
[ View ]
#16 interdiff.txt1.06 KBkbasarab
#14 drupal-1798944-14.patch9.35 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 42,126 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#14 interdiff.txt2.72 KBtim.plunkett
#9 config_test-1798944-9.patch9.55 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] 42,110 pass(es), 6 fail(s), and 24 exception(s).
[ View ]
#9 interdiff.txt3.46 KBkbasarab
#5 config_test-1798944-5.patch9.24 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] 42,105 pass(es), 4 fail(s), and 24 exception(s).
[ View ]
#3 config_test-1798944-3.patch9.75 KBkbasarab
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config_test-1798944-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Issue tags:+#pnx-sprint

Tagging

Example of what happened in entity_test of this type of conversion is available at:
http://drupal.org/files/et-form_controller-1757232-17.patch

Status:Active» Needs review
StatusFileSize
new9.75 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config_test-1798944-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should give at the least 3 fails. Posting it though because I'm not sure if I'm going at this the right way or not. First crack looking into these Entity Controllers.

I do keep getting this message:
"Declaration of Drupal\config_test\ConfigTestFormController::form() should be compatible with that of Drupal\Core\Entity\EntityFormControllerNG::form()"

Which I think is causing the page title test portions to fail.

But letting the bot hit it to get us started...

Status:Needs review» Needs work

The last submitted patch, config_test-1798944-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.24 KB
FAILED: [[SimpleTest]]: [MySQL] 42,105 pass(es), 4 fail(s), and 24 exception(s).
[ View ]

Stupid Mac files. Rerolled...

+++ b/core/modules/config/tests/config_test/config_test.infoundefined
@@ -2,4 +2,4 @@ name = Configuration test module
-hidden = TRUE
+hidden = FALSE

please revert this back

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -182,79 +177,20 @@ function config_test_list_page() {
+  drupal_set_title(t('Create a config_test'));

No reason, menu_item already provides one

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -182,79 +177,20 @@ function config_test_list_page() {
+  drupal_set_title(t('config_test @id', array('@id' => $entity->id)), PASS_THROUGH);

Use $entity->label() here.
Not sure that t() needed here, this is a test module.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -0,0 +1,104 @@
+      '#default_value' => $entity->label(),

Controller should know entity fields also label() could return translated value...

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -0,0 +1,104 @@
+    $message = $is_new ? t('config_test @id has been created.', array('@id' => $entity->id())) : t('config_test @id has been updated.', array('@id' => $entity->id()));
...
+    drupal_set_message(t('config_test @id has been deleted.', array('@id' => $entity->id())));

Messages should contain label() not id()
Probably tests require some fixes

Status:Needs review» Needs work

The last submitted patch, config_test-1798944-5.patch, failed testing.

Assigned:Unassigned» kbasarab

Thanks andypost. I'll take a look at those this weekend or early next week.

StatusFileSize
new3.46 KB
new9.55 KB
FAILED: [[SimpleTest]]: [MySQL] 42,110 pass(es), 6 fail(s), and 24 exception(s).
[ View ]

Fixed the items AndyPost was mentioning.

Though I didn't understand this one as the other items in this form have default_value's also:

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -0,0 +1,104 @@
+ '#default_value' => $entity->label(),

Controller should know entity fields also label() could return translated value...

Also not sure why my tests are failing on: "Declaration of Drupal\config_test\ConfigTestFormController::form() should be compatible with that of Drupal\Core\Entity\EntityFormControllerNG::form()"

If anyone could help guide me on that item and how to try and correct that would be awesome. The actual fails in the test results are 3 and they are for Drupal titles. When I try testing visually via module I get "Error" as my page title and the only thing I can think is from those warnings. Any help would be appreciated.

@kbasarab take a look at patch provided by @tim.plunkett #1588422-133: Convert contact categories to configuration system

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -117,15 +115,14 @@ function config_test_menu() {
   $items['admin/structure/config_test/manage/%config_test'] = array(
     'title' => 'Edit test configuration',
-    'page callback' => 'drupal_get_form',
-    'page arguments' => array('config_test_form', 4),
+    'page callback' => 'config_test_edit',
+    'page arguments' => array(4),
     'access callback' => TRUE,
   );
   $items['admin/structure/config_test/manage/%config_test/edit'] = array(

Any reason to have 2 menu callbacks?

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -0,0 +1,104 @@
+      '#default_value' => $entity->label(),

This should be $entity->label;

EDIT It looks like most of this already in core
See core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test folder

Any reason to have 2 menu callbacks?

The second is a MENU_DEFAULT_LOCAL_TASK, that's fine.

This should be $entity->label;

No it shouldn't :)

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, config_test-1798944-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.72 KB
new9.35 KB
FAILED: [[SimpleTest]]: [MySQL] 42,126 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Here's a bunch of fixes, they're mostly reverts.

Status:Needs review» Needs work

The last submitted patch, drupal-1798944-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.06 KB
new10.41 KB
PASSED: [[SimpleTest]]: [MySQL] 42,179 pass(es).
[ View ]

So here is a patch that passes but I want to get clarification from someone on what I've done. The failures above were located within

core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.php

The portion I removed was there to "verify the appropriate module API hooks have been invoked."

Since this is now using EntityFormController if I'm not mistaken wouldn't these API hooks be tested during the EntityCrudHookTestCase?

StatusFileSize
new1.96 KB
new11.03 KB
FAILED: [[SimpleTest]]: [MySQL] 42,173 pass(es), 10 fail(s), and 0 exception(s).
[ View ]

Removes reference to hooks file and the require once in config_test.module to further remove the need for testing API hooks since that should now be done by EntityFormController and the EntityCrudHookTestCast.

Status:Needs review» Needs work

I agree, those shouldn't be needed here.

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -95,6 +80,19 @@ function config_test_entity_info() {
+ * Implements hook_permission().
+ */
+function config_test_permission() {
+  $permissions = array(
+    'administer config_test content' => array(
+      'title' => t('Administer entity_test content'),
+      'description' => t('Manage config_test contnet'),
+    )
+  );
+  return $permissions;

I don't see this being used, why bother adding it?

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -0,0 +1,105 @@
+<?php
+/**
+ * @file

There should be a space after the php tag.

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -0,0 +1,105 @@
+class ConfigTestFormController extends EntityFormControllerNG {

I don't see the reason to use EntityFormControllerNG instead of EntityFormController, its a compatibility layer for fieldable entities, which config_test isn't.

Status:Needs work» Needs review
StatusFileSize
new5.32 KB
new14.23 KB
PASSED: [[SimpleTest]]: [MySQL] 42,162 pass(es).
[ View ]

hook_perm was implemented because of the example I was basing off of. Removed now.

Added space between opening php statement and @file block.

Changed EntityFormContrllerNG to use EntityFormController. Was originally added trying to debug the spelling error of interface.

Also found quite a few other references to the hook testing. Those have been removed as well.

Assigned:kbasarab» sun

Coming from: #1588422: Convert contact categories to configuration system

I've no idea why you're deleting lots of required/essential test coverage here; I guess that must have been a bogus re-roll/merge or something.

Anyway, I'm working on this. Essential patch is done, but I'm adding some serious test coverage for ConfigEntity CRUD operations, so as to ensure that the form controller (and any other code) gets and sees the right properties and return values.

StatusFileSize
new26.83 KB
new26.36 KB
PASSED: [[SimpleTest]]: [MySQL] 42,895 pass(es).
[ View ]

Reverted bogus removal of config import support code and test coverage.
Added ConfigEntiy CRUD test coverage; fixed bugs.

Awesome! Just a few questions:

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -45,23 +46,21 @@ public function getOriginalID() {
+  public function setOriginalID($id) {
+    $this->originalID = $id;
   }

Maybe better make it in Entity?

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -295,6 +309,9 @@ public function save(EntityInterface $entity) {
       $config->save();
       $this->postSave($entity, TRUE);
       $this->invokeHook('update', $entity);
+
+      // Immediately update the original ID.
+      $entity->setOriginalID($entity->id());

So getOriginalID() could be used when machine name is changed?

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -182,79 +161,28 @@ function config_test_list_page() {
+  drupal_set_title(t('Edit %label', array('%label' => $config_test->label())), PASS_THROUGH);

Not sure that usage of t() good for test module, but this makes sense as common pattern

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTestFormController.phpundefined
@@ -0,0 +1,90 @@
+      '#machine_name' => array(
+        'exists' => 'config_test_load',
+        // @todo Update form_process_machine_name() to use 'label' by default.
+        'source' => array('label'),

Is there issue for that? Are you sure that process* function should detect entityForms?

Maybe better make it in Entity?

Nope, getOriginalID() only exists on ConfigEntityBase/ConfigEntityInterface. And so does setOriginalID().

So getOriginalID() could be used when machine name is changed?

During saving, yes. (i.e., methods and hooks being invoked could do that.) ConfigStorageController only updates the originalID as last step, after the config entity was saved.

Not sure that usage of t() good for test module, but this makes sense as common pattern

That's a good point - so far, config_test.module does not use t() anywhere and so this page title shouldn't use t() either.

Is there issue for that? Are you sure that process* function should detect entityForms?

There was no issue for it yet, but now there is:
#1820834: Change #type machine_name default 'source' element to 'label'

StatusFileSize
new26.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.test-form.24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Replaced t() with format_string().

Status:Needs review» Needs work

The last submitted patch, config.test-form.24.patch, failed testing.

It seems something in this tests is fragile, or introduces random failures

head broken in 4365f50..0b70f27

Status:Needs work» Needs review
Issue tags:-Entity system, -Configuration system, -Configurables, -#pnx-sprint

#24: config.test-form.24.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Entity system, +Configuration system, +Configurables, +#pnx-sprint

The last submitted patch, config.test-form.24.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.15 KB
FAILED: [[SimpleTest]]: [MySQL] 46,274 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Merged HEAD.

Status:Needs review» Needs work

The last submitted patch, config.test-form.29.patch, failed testing.

@tim.plunkett - Suppose this change is not appropriate to views, please comment about

The views test assumes the following (where 'name' is currently our id_key):

$view = entity_load('view', 'foo');
$view->set('name', 'foo_copy');
$view->save();
$old = config('views.view.foo');
$new = config('views.view.foo_copy');
!empty($old) == TRUE
!empty($new) == TRUE

That is, changing the ID and saving is a copy, not a rename. If that's not supposed to be the case, then the test needs to be updated (ViewStorageTest::saveTests())

Status:Needs work» Needs review
StatusFileSize
new3.31 KB
new29.19 KB
PASSED: [[SimpleTest]]: [MySQL] 46,321 pass(es).
[ View ]

So I starred at this code for quite some while, and even had to patch Simpletest in order to figure out which exact operation in eh test is throwing that entity exception...

...only to realize that this entire test method does nothing else than duplicating tests for bare configurable/configuration system functionality, and has absolutely no business in doing so.

Tests that are duplicating test coverage are one of the worst offenders in testing. There are situations in which one cannot prevent it; e.g., implicitly testing the router system, redirect behavior, and other facilities that happen to be part of the test execution or its assertions. However, whenever a test or test method factually duplicates existing, dedicated test coverage elsewhere, that is a bug.

This is the case here. The test loads a Configurable, changes entity properties, saves it, recreates it from bare config, saves it under a different ID, and reloads it as entity. Nothing else is tested. This test method has absolutely nothing to do Views functionality.

Thus:

Removed off-topic test coverage from ViewStorageTest.
Added ConfigEntityBase::createDuplicate() override for extra safety.

Status:Needs review» Reviewed & tested by the community

Those tests methods were present all through our port, even before ConfigEntities, just to ensure Views could do what we needed it to.
I'm definitely okay with removing it now.

And the createDuplicate() change looks good.

Pre-emptive RTBC if it's green.

#33: config.test-form.34.patch queued for re-testing.

This is still blocking the follow-up fixes in #1588422: Convert contact categories to configuration system

Status:Reviewed & tested by the community» Needs work

This no longer applies unfortunately. I found a couple of minor things but otherwise it looks good.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -32,8 +32,9 @@ public function __construct(array $values, $entity_type) {
     // Backup the original ID, if any.
-    if ($original_id = $this->id()) {
-      $this->originalID = $original_id;
+    $original_id = $this->id();
+    if ($original_id !== NULL && $original_id !== '') {
+      $this->setOriginalID($original_id);

This could do with an explantion why we're explicitly checking for empty strings/NULL (as opposed to empty or similar).

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.phpundefined
@@ -45,23 +46,21 @@ public function getOriginalID() {
+  final public function isNew() {
+    return !empty($this->enforceIsNew) || $this->id() === NULL || $this->id() === '';
   }

That's the same check more or less. Also how can an ID be passed that's actually an empty string or NULL?

I actually ran into an issue with this on a Drupal 6 client site - there was code setting $node->nid = NULL when creating new nodes, and that resulted in deleting the node_acess record with nid = 0 or similar - but it was horribly broken code and throwing an exception there might have been friendlier.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -222,6 +223,12 @@ public function create(array $values) {
+    // @todo If we expect isNew() to return FALSE when given an ID that exists
+    //   already, wrap this in !config($prefix . $id)->getStorage()->exists().
+    //   In that case, do we expect existing values to be merged into $values?

Is there an issue open for the @todo?

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.38 KB
new28.8 KB
PASSED: [[SimpleTest]]: [MySQL] 46,383 pass(es).
[ View ]
  1. Merged HEAD.
  2. Added inline comments to explain the strict config entity ID conditions.
  3. Removed @todo from ConfigStorageController::create().

Back to RTBC.

Status:Reviewed & tested by the community» Fixed

I didn't review this overly thoroughly, but it looks like catch did already, and his feedback was addressed.

Committed and pushed to 8.x. Thanks!

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