Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comments
Comment #1
andypostIt should be easy find/replace and extend tests
Comment #2
tsphethean CreditAttribution: tsphethean commentedAn initial stab at this - getting a little bit stuck on writing a unit test for it so going to keep digging, any pointers welcome!
Comment #3
andypostGreat! let's check other implementations use
parent::buildRow()
and thenunset()
Comment #4
andypostLet's get this in and continue in #2027117: Always use parent::buildRow($entity) in EntityListController
Comment #5
andypostAlso related #1855402: Add generic weighting (tabledrag) support for config entities (DraggableListController)
Comment #6
tsphethean CreditAttribution: tsphethean commentedI'm writing tests for this today.
Comment #7
tsphethean CreditAttribution: tsphethean commentedThis 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
Comment #8
tsphethean CreditAttribution: tsphethean commentedManaged 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.
Comment #9
tsphethean CreditAttribution: tsphethean commentedAdded some mocking to the unit tests to remove the dependency on common.inc. Tests passing locally, hopefully good to go.
Comment #10
tsphethean CreditAttribution: tsphethean commentedTests passing locally now.
Comment #11
tsphethean CreditAttribution: tsphethean commentedTests added and passing locally.
Comment #12
tsphethean CreditAttribution: tsphethean commentedTest added.
Comment #13
tsphethean CreditAttribution: tsphethean commentednot sure what was going on there - patches werent showing as uploaded and then all of a sudden there were 5!
Comment #14
tsphethean CreditAttribution: tsphethean commentedPatch in #12 is the one to go for review (identical to #11, but prob easier to grab the last patch
Comment #14.0
tsphethean CreditAttribution: tsphethean commentedUpdated issue summary.
Comment #15
dawehnerIt would be possible to use data providers to test different kind of labels.
Comment #16
tsphethean CreditAttribution: tsphethean commented@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.
Comment #18
tsphethean CreditAttribution: tsphethean commentedHmm. 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.
Comment #19
dawehner#16: entitylistcontroller_checkplain-2019071-16.patch queued for re-testing.
Comment #21
tsphethean CreditAttribution: tsphethean commented#16: entitylistcontroller_checkplain-2019071-16.patch queued for re-testing.
Re-queueing as follow up raised - see #2032453: WebTestBase::randomString returning a string containing a $ followed by a number causes assertLink() to fail
Comment #22
dawehnerWOW!
Comment #23
tsphethean CreditAttribution: tsphethean commented@dawehner Is that a good wow?
Comment #24
dawehnerYes it is.
Comment #25
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #26
alexpottWe can do all of this in setUp() and add a protected $entityListController property to use in the test.
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...Comment #27
tsphethean CreditAttribution: tsphethean commentedThanks 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?
Comment #28
tim.plunkettThanks!
Comment #29
alexpottActually let's not share dataproviders... this is creating an unneeded and difficult to spot dependency on StringTest::providerCheckPlain()
Comment #30
tsphethean CreditAttribution: tsphethean commentedNo 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.
Comment #31
dawehnerwe kind of have a code-pattern to name the providers which is like providerTestBuildRow in this case.
let's use self::testBuildRow()
let's just remove the empty line.
Comment #32
tsphethean CreditAttribution: tsphethean commentedAll comments from #31 addressed in this patch - thanks @dawehner.
Comment #34
tsphethean CreditAttribution: tsphethean commentedOdd. All these tests pass in my local. Some of the failures mention out of disk space. could testbot be sick?
Comment #35
Berdir#32: entitylistcontroller_checkplain-2019071-32.patch queued for re-testing.
Comment #36
BerdirYes, 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
Comment #37
tsphethean CreditAttribution: tsphethean commentedThanks Berdir
Comment #38
dawehnerWhat I really like is to have a @see for the class which is tested.
We sadly have to document this quite easy to read variables.
missing @return :(
Comment #39
tsphethean CreditAttribution: tsphethean commentedThanks for the review.
Done
Done
Done :-)
Comment #40
dawehner+1 for actually using the groups provided by PHPUnit.
Comment #41
catchCommitted/pushed to 8.x, thanks!
Comment #42.0
(not verified) CreditAttribution: commentedUpdated issue summary.