Entity form controllers already have a fair test coverage, since every real controller extending the base class has test coverage. However we need a form controller also for the test entity in order to ensure the generic logic is sane and functional. Moreover this is needed to provide proper test coverage for the upcoming Entity Translation UI that has no notion of real entities and thus has to work with the test one. As a bonus this will allow to test the UI with multilingual properties, which currently only the test entity storage controller supports.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Active » Needs review

See commit c6725b5 and the whole topic branch.

plach’s picture

Issue tags: +Entity forms

Tagging

plach’s picture

Title: Add form controller to entity test » Improve test coverage for the entity form controller

Better title

plach’s picture

Project: D8MI - Entity Translation » Drupal core
Issue summary: View changes

Updated issue summary

plach’s picture

Issue summary: View changes

Updated issue summary.

plach’s picture

Version: » 8.x-dev
Component: Code » entity system
Status: Needs review » Active

Moving to the core queue since this is a prerequisite for the work on the Entity Translation UI.

We need a patch from the topic branch here, back to active meanwhile.

plach’s picture

Issue tags: +D8MI, +sprint, +language-content

More tags

plach’s picture

Priority: Normal » Major

Bumping to major since this is a blocker for the first UI patch.

plach’s picture

Issue summary: View changes

Updated issue summary.

peximo’s picture

The attached patch implements basic entity test page callbacks for CRUD operations and the related form controller.
It also adds some tests for this functions.

peximo’s picture

Status: Active » Needs review
plach’s picture

Status: Needs review » Reviewed & tested by the community

Simple and straightforward.

plach’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, just realized that the delete page callback is not used and needs to be removed:

+++ b/core/modules/entity/tests/modules/entity_test/entity_test.module
@@ -28,6 +31,68 @@ function entity_test_entity_info() {
+  $items['entity-test/manage/%entity_test/delete'] = array(
+    'title' => 'Delete test entity',
+    'page callback' => 'entity_test_delete',
+    'page arguments' => array(2),
+    'access arguments' => array('administer entity_test content'),
+    'type' => MENU_NORMAL_ITEM,
+  );

Since we are rolling a new patch can we perform the minor/cosmetic changes below?

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityFormTest.php
@@ -0,0 +1,77 @@
+    entity_get_controller($entity_type)->resetCache();

It would be nice to add a comment explaining why we need this, something like:
"Always load the entity from the database to ensure that changes are correctly picked up."

+++ b/core/modules/entity/tests/modules/entity_test/entity_test.module
@@ -28,6 +31,68 @@ function entity_test_entity_info() {
+  drupal_set_title(t('Create an entity_test'), PASS_THROUGH);

Can we move this line up? Also, the PASS_THROUGH should not be needed here, since there is no HTML in the passed string.

peximo’s picture

Status: Needs work » Needs review
FileSize
7.9 KB

Modified as suggested.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work

This no longer applies after the entity.module > \Drupal\Core\Entity move.

plach’s picture

Assigned: peximo » plach

Rerolling

fago’s picture

+    $entity->save();
+
+    $message = $entity->isNew() ? t('entity_test @id has been created.', array('@id' => $entity->id())) : t('entity_test @id has been updated.', array('@id' => $entity->id()));

After saving $entity->isNew() will never return TRUE, so this won't work as intended.

plach’s picture

Rerolled and fixed #15 (the interdiff covers only the latter change).

plach’s picture

Forgot to adjust some PHP docs.

plach’s picture

Status: Needs review » Reviewed & tested by the community

I think #17 should be RTBC again.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Looks good!

Committed and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Thanks, removing from the sprint.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.