Appears to be a very small and simple mistake:

LegacyMessenger::deleteByType calls MessengerInterface::messagesByType() instead of MessengerInterface::deleteByType().

CommentFileSizeAuthor
#11 2952634-11.patch2.29 KBalexpott
#11 2952634-11.test-only.patch916 bytesalexpott
#6 user_registrationpassword_without_patch.png71.08 KBRob C
#4 2952634-4.patch1.56 KBSut3kh
#2 2952634-2.patch608 bytesSut3kh

Comments

Sut3kh created an issue. See original summary.

Sut3kh’s picture

Sut3kh commented
Status: Active » Needs review
StatusFileSize
new 2952634-2.patch608 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 2952634-2.patch, failed testing. View results

Sut3kh’s picture

Sut3kh commented
Status: Needs work » Needs review
StatusFileSize
new 2952634-4.patch1.56 KB

OK from what I can tell the failure is that it is now (correctly) only finding 1 error because _file_save_upload_from_form() pulls any error messages from the messenger and into form errors. It was passing before the patch because drupal_get_messages('error') wasn't clearing the error before also printing it in a form error.

Before patch:

<div role="contentinfo" aria-label="Error message" class="messages messages--error">
  <div role="alert">
    <h2 class="visually-hidden">Error message</h2>
      <ul class="messages__list">
        <li class="messages__item">An error message set before _file_save_upload_from_form()</li>
        <li class="messages__item">The specified file <em class="placeholder">image-test.png</em> could not be uploaded.<div class="item-list"><ul><li>Only files with the following extensions are allowed: <em class="placeholder">foo</em>.</li></ul></div></li>
        <li class="messages__item">Epic upload FAIL!</li>
        <li class="messages__item">One or more files could not be uploaded.<div class="item-list"><ul><li>An error message set before _file_save_upload_from_form()</li><li>The specified file <em class="placeholder">image-test.png</em> could not be uploaded.<div class="item-list"><ul><li>Only files with the following extensions are allowed: <em class="placeholder">foo</em>.</li></ul></div></li></ul></div></li>
    </ul>
  </div>
</div>

After patch:

<div role="contentinfo" aria-label="Error message" class="messages messages--error">
  <div role="alert">
    <h2 class="visually-hidden">Error message</h2>
      <ul class="messages__list">
        <li class="messages__item">An error message set before _file_save_upload_from_form()</li>
        <li class="messages__item">Epic upload FAIL!</li>
        <li class="messages__item">The specified file <em class="placeholder">image-test.png</em> could not be uploaded.<div class="item-list"><ul><li>Only files with the following extensions are allowed: <em class="placeholder">foo</em>.</li></ul></div></li>
    </ul>
  </div>
</div>

Therefore using the 'after' count to test that error messages are preserved is no longer possible but I think this should be satisfied by the '$this->assertText($error);' on line 420 anyway.

dandaman’s picture

dandaman commented

Applied this patch to a site and it did remove the messages of type status as expected where before it was not being removed when executing deleteByType().

Rob C’s picture

Rob C commented
Status: Needs review » Reviewed & tested by the community
Related issues: +#2278383: Create an injectible service for drupal_set_message()
StatusFileSize
new user_registrationpassword_without_patch.png71.08 KB

While the user_registrationpassword tests do not trip over this, this patch includes an assertRaw() on the number of messages. Great, but is this the only place we (can) test this? The patch indeed works like expected.

This is 8.5.0 without patch / with user_registrationpassword enabled / submit the registration form: (the module replaces the first message)
without patch

With the patch the first message does not appear.

And i wonder if this should be a major issue: we get more than we expect + indeed seems like a mistake / looked over.

Updating to RTBC to get some attention.

Not 100% sure, but this prolly was introduced in #2278383: Create an injectible service for drupal_set_message(). Adding a message there + linking in, so we can get this fixed asap.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Thanks, for reporting and creating the patch.

+++ b/core/lib/Drupal/Core/Messenger/LegacyMessenger.php
@@ -180,7 +180,7 @@ public function deleteAll() {
-      return $messenger->messagesByType($type);
+      return $messenger->deleteByType($type);

We should add explicit tests for this change.

simgui8’s picture

simgui8 commented

Thanks @Sut3kh

\Drupal::messenger()->deleteByType($type) was deleting nothing for me in Drupal 8.5.1

#4 fixed it

nottaken’s picture

nottaken commented

Patch in #4 worked for me as well on 8.5.3. Thanks Sut3kh

thejacer87’s picture

thejacer87 commented
Status: Needs work » Reviewed & tested by the community

confirmed working on 8.5.5

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.5.x-dev » 8.6.x-dev
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2952634-11.test-only.patch916 bytes
new 2952634-11.patch2.29 KB

Adding a test. 

+++ b/core/modules/file/src/Tests/SaveUploadFormTest.php
@@ -419,7 +419,10 @@ public function testErrorMessagesAreNotChanged() {
+    // NOTE: We actually expect 2 errors at the end but we only assert 1 error
+    // because when the messages are counted, _file_save_upload_from_form() has
+    // moved the errors from the messenger into form errors.

Removed this comment because it is not necessary - in fact whilst doing #2942068: FileTestSaveUploadFromForm incorrectly counts messages we should have noticed that deleteByType is not working. There's no case in which the messages should increase because the whole point of _file_save_upload_from_form() is to move messages to form errors.

No interdiff because this is also a re-roll for 8.6.x. We need to apply the patch to 8.6.x / 8.7.x first.

Status: Needs review » Needs work

The last submitted patch, 11: 2952634-11.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Reviewed & tested by the community

Test only patch failed. RTBC if #11 comes back green on 8.6.x.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed 7353b6f170 to 8.7.x and f2d09530f4 to 8.6.x and cbe0ec9 and to 8.5.x. Thanks!

  • alexpott committed 7353b6f on 8.7.x 
    Issue #2952634 by Sut3kh, alexpott, Rob C, jibran: LegacyMessenger::...

  • alexpott committed f2d0953 on 8.6.x 
    Issue #2952634 by Sut3kh, alexpott, Rob C, jibran: LegacyMessenger::...

  • alexpott committed cbe0ec9 on 8.5.x 
    Issue #2952634 by Sut3kh, alexpott, Rob C, jibran: LegacyMessenger::...

  • alexpott committed 4fdf725 on 8.5.x 
    Revert "Issue #2952634 by Sut3kh, alexpott, Rob C, jibran:...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Reverted from 8.5.x since 8.5.x is in criticals only. This will be fixed in 8.6.0.

dawehner’s picture

dawehner
Primary language German
commented

@Sut3kh I'm wondering whether you could describe how you encountered this problem. Is this something which was a problem on your site / your contrib module or did you just read the code itself?

Sut3kh’s picture

Sut3kh commented

@dawehner I was updating some code that improves the node save messages to use \Drupal::messenger() for 8.5 (used to manipulate $_SESSION directly).

It grabs all status messages, deletes them (deleteByType) and then adds them again after parsing through regex.

When I saw that i was still getting the original messages as well, stepping through \Drupal::messenger()->deleteByType() showed where the issue was.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.