Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

Issue tags: +#pnx-sprint

Tagging

kbasarab’s picture

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

kbasarab’s picture

Status: Active » Needs review
FileSize
9.75 KB

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.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
9.24 KB

Stupid Mac files. Rerolled...

andypost’s picture

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

kbasarab’s picture

Assigned: Unassigned » kbasarab

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

kbasarab’s picture

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.

andypost’s picture

@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

tim.plunkett’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.72 KB
9.35 KB

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.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
1.06 KB
10.41 KB

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?

kbasarab’s picture

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.

tim.plunkett’s picture

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.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
5.32 KB
14.23 KB

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.

sun’s picture

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.

sun’s picture

FileSize
26.83 KB
26.36 KB

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

andypost’s picture

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?

sun’s picture

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'

sun’s picture

FileSize
26.38 KB

Replaced t() with format_string().

Status: Needs review » Needs work

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

andypost’s picture

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

head broken in 4365f50..0b70f27

andypost’s picture

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.

sun’s picture

Status: Needs work » Needs review
FileSize
26.15 KB

Merged HEAD.

Status: Needs review » Needs work

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

andypost’s picture

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

tim.plunkett’s picture

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

sun’s picture

Status: Needs work » Needs review
FileSize
3.31 KB
29.19 KB

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.

tim.plunkett’s picture

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.

sun’s picture

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

sun’s picture

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

catch’s picture

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?

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.38 KB
28.8 KB
  1. Merged HEAD.
  2. Added inline comments to explain the strict config entity ID conditions.
  3. Removed @todo from ConfigStorageController::create().

Back to RTBC.

webchick’s picture

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.