Follow-up from #1809376-31: Use EntityListController for image styles

Problem/Motivation

Currently label passed as is so each controller needs to make check_plain()

Proposed resolution

Apply check_plain in EntityListController::buildRow() for label
Add test for default implementation

Follow ups

Files: 
CommentFileSizeAuthor
#39 entitylistcontroller_checkplain-2019071-39.patch6.85 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 57,625 pass(es).
[ View ]
#32 entitylistcontroller_checkplain-2019071-32.patch6.41 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 57,808 pass(es).
[ View ]
#30 entitylistcontroller_checkplain-2019071-30.patch6.4 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 57,907 pass(es).
[ View ]
#30 27-30-interdiff-do-not-test.patch2.85 KBtsphethean
#27 entitylistcontroller_checkplain-2019071-27.patch5.8 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 57,013 pass(es).
[ View ]
#16 entitylistcontroller_checkplain-2019071-16.patch5.47 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 56,773 pass(es).
[ View ]
#12 entitylistcontroller_checkplain-2019071-8.patch4.9 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 58,410 pass(es).
[ View ]
#11 entitylistcontroller_checkplain-2019071-8.patch4.9 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 58,771 pass(es).
[ View ]
#10 entitylistcontroller_checkplain-2019071-7.patch1.97 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entitylistcontroller_checkplain-2019071-7_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 entitylistcontroller_checkplain-2019071-7.patch1.97 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entitylistcontroller_checkplain-2019071-7_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 entitylistcontroller_checkplain-2019071-7.patch1.97 KBtsphethean
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entitylistcontroller_checkplain-2019071-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 entitylistcontroller_checkplain-2019071-2.patch2.07 KBtsphethean
PASSED: [[SimpleTest]]: [MySQL] 58,078 pass(es).
[ View ]

Comments

Issue tags:+Novice

It should be easy find/replace and extend tests

Status:Active» Needs review
StatusFileSize
new2.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,078 pass(es).
[ View ]

An initial stab at this - getting a little bit stuck on writing a unit test for it so going to keep digging, any pointers welcome!

Great! let's check other implementations use parent::buildRow() and then unset()

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

I'm writing tests for this today.

This issue depends on #2028499: drupal_sort_weight should be converted to a class in order to be able to run unit tests on the EntityListController class

StatusFileSize
new1.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entitylistcontroller_checkplain-2019071-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Managed to get mocking working so that this no longer depends on drupal_sort_weight in common.inc and so should be able to go independently.

Status:Needs work» Needs review
StatusFileSize
new1.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entitylistcontroller_checkplain-2019071-7_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Added some mocking to the unit tests to remove the dependency on common.inc. Tests passing locally, hopefully good to go.

StatusFileSize
new1.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch entitylistcontroller_checkplain-2019071-7_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Tests passing locally now.

StatusFileSize
new4.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,771 pass(es).
[ View ]

Tests added and passing locally.

StatusFileSize
new4.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,410 pass(es).
[ View ]

Test added.

not sure what was going on there - patches werent showing as uploaded and then all of a sudden there were 5!

Patch in #12 is the one to go for review (identical to #11, but prob easier to grab the last patch

Issue summary:View changes

Updated issue summary.

It would be possible to use data providers to test different kind of labels.

StatusFileSize
new5.47 KB
PASSED: [[SimpleTest]]: [MySQL] 56,773 pass(es).
[ View ]

@dawehner - good point, I've updated the tests to use the data provider from StringTest::providerCheckPlain so that if the String::checkPlain method is ever updated then we'll catch any regressions here.

Status:Needs review» Needs work

The last submitted patch, entitylistcontroller_checkplain-2019071-16.patch, failed testing.

Assigned:Unassigned» tsphethean

Hmm. So looks like this is something to do with a particular randomString being assigned to the role name when it is generated in simpletest. None of the characters should cause check plain to invalidate. Trying to get a repeatable failure in my local to eliminate this, as I suspect that re-queuing for testing will turn this green without us actually figuring out why and will potentially lead to random failures in future tests.

I'll keep working on this.

Status:Needs work» Needs review
Issue tags:-Needs tests, -Novice

Status:Needs review» Needs work

The last submitted patch, entitylistcontroller_checkplain-2019071-16.patch, failed testing.

Status:Needs review» Reviewed & tested by the community

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,111 @@
+   * @dataProvider Drupal\Tests\Component\Utility\StringTest::providerCheckPlain
...
+  public function testBuildRow($input, $expected, $message, $ignorewarnings = FALSE) {

WOW!

@dawehner Is that a good wow?

Yes it is.

Issue tags:-Needs tests+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Status:Reviewed & tested by the community» Needs work

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,111 @@
+    // Creates a stub role storage controller and replace the attachLoad()
+    // method with an empty version, because attachLoad() calls
+    // module_implements().
+    $role_storage_controller = $this->getMockBuilder('Drupal\user\RoleStorageController')
+      ->disableOriginalConstructor()
+      ->getMock();
+
+    $module_handler = $this->getMockBuilder('Drupal\Core\Extension\ModuleHandler')
+      ->disableOriginalConstructor()
+      ->getMock();
+
+    $entity_list_controller = $this->getMock('Drupal\Core\Entity\EntityListController', array('buildOperations'), array('user_role', static::$entityInfo, $role_storage_controller, $module_handler));
+
+    $entity_list_controller->expects($this->any())
+      ->method('buildOperations')
+      ->will($this->returnValue(array()));

We can do all of this in setUp() and add a protected $entityListController property to use in the test.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,111 @@
+    $this->assertEquals($built_row['label'], $expected, $message);

I wonder about the message here... just using the message from the dataprovider seems odd. As we'll get a messages like String::checkPlain() rejects invalid sequence "Foo\\xC0barbaz" see the messages below...

phpunit --filter EntityListControllerTest --tap
TAP version 13
ok 1 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #0 ('Foo�barbaz', '', 'String::checkPlain() rejects invalid sequence "Foo\\xC0barbaz"', true)
ok 2 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #1 ('�"', '', 'String::checkPlain() rejects invalid sequence "\\xc2""', true)
ok 3 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #2 ('Fooÿñ', 'Fooÿñ', 'String::checkPlain() accepts valid sequence "Fooÿñ"')
ok 4 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #3 ('<script>', '&lt;script&gt;', 'String::checkPlain() escapes &lt;script&gt;')
ok 5 - Drupal\Tests\Core\Entity\EntityListControllerTest::testBuildRow with data set #4 ('<>&"\'', '&lt;&gt;&amp;&quot;&#039;', 'String::checkPlain() escapes reserved HTML characters.')
1..5

Status:Needs work» Needs review
StatusFileSize
new5.8 KB
PASSED: [[SimpleTest]]: [MySQL] 57,013 pass(es).
[ View ]

Thanks for the feedback - updated patch moves set up logic to setup() as suggested.

On the messages, agree they aren't ideal so have added additional statement so it's more obvious what is happening when a test fails. Is there still benefit in re-using the data provider from StringTest::providerCheckPlain or do you think it's more appropriate duplicating the data provider in favour of better messages?

Category:task» bug
Priority:Normal» Major
Status:Needs review» Reviewed & tested by the community

Thanks!

Status:Reviewed & tested by the community» Needs work

Actually let's not share dataproviders... this is creating an unneeded and difficult to spot dependency on StringTest::providerCheckPlain()

Status:Needs work» Needs review
StatusFileSize
new2.85 KB
new6.4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,907 pass(es).
[ View ]

No problem (you're right - the feedback messages do read better now theyre in their own data provider...). New patch attached and what I think is a correctly generated interdiff.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -80,24 +80,22 @@ protected function setUp() {
+   * @dataProvider providerBuildRow

we kind of have a code-pattern to name the providers which is like providerTestBuildRow in this case.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -110,11 +108,28 @@ public function testBuildRow($input, $expected, $check_plain_message, $ignorewar
+   * @see testBuildRow()

let's use self::testBuildRow()

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -110,11 +108,28 @@ public function testBuildRow($input, $expected, $check_plain_message, $ignorewar
+

let's just remove the empty line.

StatusFileSize
new6.41 KB
PASSED: [[SimpleTest]]: [MySQL] 57,808 pass(es).
[ View ]

All comments from #31 addressed in this patch - thanks @dawehner.

Status:Needs review» Needs work

The last submitted patch, entitylistcontroller_checkplain-2019071-32.patch, failed testing.

Odd. All these tests pass in my local. Some of the failures mention out of disk space. could testbot be sick?

Status:Needs work» Needs review

Yes, testbots can get sick. If you see errors like that, try a re-test. And check the testbot number in the event log (this was #953) and ask in IRC if somoene can check it and/or trigger a re-test of the bot to have it auto-disable

Thanks Berdir

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,134 @@
+ * Tests the entity access controller.
...
+class EntityListControllerTest extends UnitTestCase {

What I really like is to have a @see for the class which is tested.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,134 @@
+  /**
...
+  protected $role;
...
+  /**
...
+  protected $entityListController;

We sadly have to document this quite easy to read variables.

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,134 @@
+  public function providerTestBuildRow() {

missing @return :(

StatusFileSize
new6.85 KB
PASSED: [[SimpleTest]]: [MySQL] 57,625 pass(es).
[ View ]

Thanks for the review.

What I really like is to have a @see for the class which is tested.

Done

We sadly have to document this quite easy to read variables.

Done

missing @return :(

Done :-)

Status:Needs review» Reviewed & tested by the community

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityListControllerTest.phpundefined
@@ -0,0 +1,148 @@
+ * @group Entity

+1 for actually using the groups provided by PHPUnit.

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

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

Issue summary:View changes

Updated issue summary.