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

andypost’s picture

Issue tags:+Novice

It should be easy find/replace and extend tests

tsphethean’s picture

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!

andypost’s picture

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

andypost’s picture

Status:Needs review» Reviewed & tested by the community
andypost’s picture

tsphethean’s picture

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

I'm writing tests for this today.

tsphethean’s picture

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

tsphethean’s picture

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.

tsphethean’s picture

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.

tsphethean’s picture

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.

tsphethean’s picture

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

Tests added and passing locally.

tsphethean’s picture

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

Test added.

tsphethean’s picture

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

tsphethean’s picture

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

tsphethean’s picture

Issue summary:View changes

Updated issue summary.

dawehner’s picture

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

tsphethean’s picture

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.

tsphethean’s picture

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.

dawehner’s picture

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.

tsphethean’s picture

dawehner’s picture

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!

tsphethean’s picture

@dawehner Is that a good wow?

dawehner’s picture

Yes it is.

YesCT’s picture

Issue tags:-Needs tests+RTBC July 1

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

alexpott’s picture

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
tsphethean’s picture

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?

tim.plunkett’s picture

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

Thanks!

alexpott’s picture

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

tsphethean’s picture

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.

dawehner’s picture

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

tsphethean’s picture

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.

tsphethean’s picture

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

Berdir’s picture

Status:Needs work» Needs review
Berdir’s picture

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

tsphethean’s picture

Thanks Berdir

dawehner’s picture

+++ 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 :(

tsphethean’s picture

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

dawehner’s picture

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.

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/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.