Messenger methods drop repeat flag

Patch is green. The test looks good. Thanks, for the quick fix.

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

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.

+1

Fixed LegacyMessenger also and added tests. Uploaded test-only patch to confirm failing tests. #6 not done yet.

+++ b/core/tests/Drupal/KernelTests/Core/Messenger/LegacyMessengerTest.php
@@ -81,4 +81,25 @@ public function testMessages() {
+  public function testRepeatedMessages() {

Minor nit, missing doc

Other than that, this looks much better :D

This is looking great, only found some tiny nits.

1. +++ b/core/tests/Drupal/KernelTests/Core/Messenger/LegacyMessengerTest.php
   @@ -81,4 +83,30 @@ public function testMessages() {
   +  public function testRepeatedMessages() {
   
   +++ b/core/tests/Drupal/KernelTests/Core/Messenger/MessengerTest.php
   @@ -0,0 +1,165 @@
   +   * Tests we don't add duplicates.
   

   Are we using "duplicated" and "repeated" to mean the same thing? Should we pick one?

2. +++ b/core/tests/Drupal/KernelTests/Core/Messenger/MessengerTest.php
   @@ -0,0 +1,165 @@
   +  public function testRemoveSingleMessage() {
   +
   ...
   +
   +  }
   ...
   +  public function testAddNoDuplicates() {
   +
   ...
   +    $this->assertCount(0, $this->messenger->messagesByType(MessengerInterface::TYPE_ERROR));
   +
   +  }
   ...
   +  public function testAddWithDuplicates() {
   +
   ...
   +
   +  }
   ...
   +  public function testAddMarkup() {
   +
   ...
   +
   +  }
   

   Nit, why are these bookended with newlines? Maybe this is just a style thing and can be ignored.

#15 are extremely minor nits

Marking RTBC, tests have passed!

Totally fair on both points! +1 RTBC

Doesn't apply?

error: core/tests/Drupal/KernelTests/Core/Messenger/LegacyMessengerTest.php: No such file or directory
error: core/tests/Drupal/KernelTests/Core/Messenger/MessengerTest.php: already exists in working directory

I think maybe you applied the interdiff instead of the patch.

Also 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

Make its class start with the Legacy prefix;

from 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

Here's a reroll, renaming existing MessengerTest (formally known as LegacyMessengerTest) to MessengerLegacyTest.

Have a couple more things to add, but decided to do that in a separate patch to create a more readable interdiff.

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

Committed as a351d72 and pushed to 8.5.x.