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 to #2760167: Add \Drupal\Core\Messenger\Messenger some of the methods on \Drupal\Core\Messenger\Messenger
don't pass through the repeat
flag.
We also need to add test coverage.
Comments
Comment #2
kim.pepperWorking on this.
Comment #3
kim.pepperAdded a unit test to cover logic in addMessage() including repeat flag.
Comment #5
jibranPatch is green. The test looks good. Thanks, for the quick fix.
Comment #6
markhalliwellI can't believe we all missed this :-/
That being said, we really should flush out a full unit test for Messenger (including all the additions, retrievals, and deletions), not just the repeat code.
Comment #7
almaudoh CreditAttribution: almaudoh commented+1
Comment #8
almaudoh CreditAttribution: almaudoh commentedFixed
LegacyMessenger
also and added tests. Uploaded test-only patch to confirm failing tests. #6 not done yet.Comment #9
kim.pepperComment #11
kim.pepperI moved the unit test to a kernel test and added a bunch of asserts from
\Drupal\Tests\system\Functional\Bootstrap\DrupalSetMessageTest::testDrupalSetMessage
and\Drupal\system_test\Controller\SystemTestController::drupalSetMessageTest
Perhaps they can be removed now?
I think we have 100% test coverage of the Messenger class now.
Comment #12
markhalliwellMinor nit, missing doc
Other than that, this looks much better :D
Comment #13
kim.pepperFixes nit in #12
Also after discussing with @markcarver in slack, added a test that an object implementing
MarkupInterface
is correctly converted toMarkup
.Also discussed removing:
\Drupal\Tests\system\Functional\Bootstrap\DrupalSetMessageTest::testDrupalSetMessage
\Drupal\system_test\Controller\SystemTestController::drupalSetMessageTest
...but think these should be done in a follow up, as there are some migration tests that depend on these, and I don't want to delay this issue on that.
Also, I managed to stuff up my local git commits, so the interdiff was created using the diff between the two patch files (facepalm).
Comment #14
kim.pepperComment #15
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedThis is looking great, only found some tiny nits.
Are we using "duplicated" and "repeated" to mean the same thing? Should we pick one?
Nit, why are these bookended with newlines? Maybe this is just a style thing and can be ignored.
Comment #16
markhalliwell#15 are extremely minor nits
Marking RTBC, tests have passed!
Comment #17
kim.pepperThanks @Sam152!
Comment #18
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedTotally fair on both points! +1 RTBC
Comment #19
larowlanDoesn't apply?
Comment #20
markhalliwellI think maybe you applied the interdiff instead of the patch.
Comment #21
larowlanAlso have to rename LegacyMessengerTest because any test with Legacy in the name generates deprecation notices that PHPUnit 6 can catch but PHPUnit 4 for some reason does not.
#2927806: Use PHPUnit 6 for testing when PHP version >= 7.2
Comment #22
larowlanComment #23
larowlanfrom http://symfony.com/blog/new-in-symfony-2-7-phpunit-bridge
So perhaps we have to name it something that has something in-front of Legacy.
MessagingLegacyTest for example
Comment #24
markhalliwellComment #25
markhalliwellHere's a reroll, renaming existing
MessengerTest
(formally known asLegacyMessengerTest
) toMessengerLegacyTest
.Have a couple more things to add, but decided to do that in a separate patch to create a more readable interdiff.
Comment #26
markhalliwellSo it turns out that we need to force repeat when merging from LegacyMessenger into the messenger service. I've updated the tests accordingly.
Also, bumping up the priority because this issue is blocking several other issues and breaks expected functionality.
Comment #27
kim.pepperAddresses #19 and #21 so back to RTBC
Comment #28
larowlanCommitted as a351d72 and pushed to 8.5.x.