Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem/Motivation
Comment Preview (Drupal\Tests\comment\Functional\CommentPreview)
✘ Comment preview
┐
├ Failed asserting that a string is an instance of interface Drupal\Component\Render\MarkupInterface.
│
│ /builds/issue/drupal-3496259/core/modules/comment/tests/src/Functional/CommentPreviewTest.php:59
┴
✔ Comment preview duplicate submission
✔ Comment edit preview save
Steps to reproduce
Example: https://git.drupalcode.org/issue/drupal-3496259/-/jobs/3829641#L1340
Proposed resolution
Replace calls to State with KeyValue.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3496319
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #4
spokjeWhilst we're only trying to fix
CommentPreviewTest::testCommentPreviewhere, this means making changes in\Drupal\user_hooks_test\Hook\UserHooksTest.In turn, those changes affect the following tests:
\Drupal\Tests\comment\Functional\CommentPreviewTest::testCommentPreview\Drupal\Tests\user\Kernel\UserEntityLabelTest::testLabelCallback\Drupal\Tests\user\Functional\UserEditTest::testUserEdit\Drupal\Tests\user\Functional\UserTokenReplaceTest::testUserTokenReplacementSo I think we have to run our multiple-run tests on all of them before and after the chage.
MR !10711 Has some pipeline changes to make those tests, and only those tests, run 2500 times and gives us 28 fails out of the 2500 runs. Those fails are all in
\Drupal\Tests\comment\Functional\CommentPreviewTest::testCommentPreviewMR !10712 shows that changing \Drupal::state()->set()/get() to Drupal::keyValue()->set()/get(); passes a 2500 times run on all four mentioned tests without errors.
The reason for MR !10712 to show up as failed is _probably_ the fact that the artifacts are too big to upload.
Sadly we can't be sure of that, because that message is snipped since the job-log is too big to keep around:
Comment #6
spokjeThe changes we want committed after review are in MR!10713
Comment #7
spokjeComment #8
smustgrave commentedBased on the other one reviewed and merged this seems like a good refactor.
Comment #9
catchOne question on the MR.
Comment #10
spokjeTried to answer in thread
Comment #11
smustgrave commentedFor 1 open thread
If you are another contributor eager to jump in, please allow the original poster @spokje at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #12
spokjeI'm not getting anywhere with the remark in the MR, so throwing it out in the open for other, bigger brains.Well, that was way harder then I remembered.
Apparently conditioning to think one way (use DI, use DI, use DI), doesn't mean it can quickly reverse (remove DI, remove DI remove DI).
Anyway. I think I've got there in the end.
Comment #14
spokjeComment #15
smustgrave commentedBelieve all feedback has been addressed on this one.
Comment #19
catchCommitted/pushed to 11.x and cherry-picked back to 11.1.x.
This doesn't apply to 11.0/10.5/10.4 due to OOP hooks, moving to 'to be ported'.
Comment #24
catchBackport looks good. Test failures are unrelated.
Comment #26
alexpottFWIW does this actually happen on 10.5.x? I think state cache collecting only was turned on by default in 11.x. - anyhows no harm in the tests being aligned.
Committed 7aa2683 and pushed to 10.5.x. Thanks!
Comment #28
quietone commentedThis broke TrackerTest on 10.5.x. Just pushed a fix.
Comment #30
catchCommitted/pushed the follow-up to 10.5.x, thanks!