As part of #2935256: Remove all usages of drupal_get_message and drupal_set_message in modules we discovered that there are two calls to drupal_get_message() which incorrectly assume the structure of the returned array.

    drupal_set_message($this->t('Number of error messages before _file_save_upload_from_form(): @count.', ['@count' => count(drupal_get_messages('error', FALSE))]));
    $file = _file_save_upload_from_form($form['file_test_upload'], $form_state, 0, $form_state->getValue('file_test_replace'));
    drupal_set_message($this->t('Number of error messages after _file_save_upload_from_form(): @count.', ['@count' => count(drupal_get_messages('error', FALSE))]));

drupal_get_messages($type) actually returns an array in the format:

[$type => $messages[$type]];

So, the above use of count() is only counting the number of keys in that array, which is 1 if there are any number of errors, or 0 if there is no array key 'error'.

CommentFileSizeAuthor
#7 2942068-7-interdiff.txt4.18 KBkim.pepper
#7 2942068-7.patch4.77 KBkim.pepper
#4 2942068-4-interdiff.txt774 byteskim.pepper
#4 2942068-4.patch2.35 KBkim.pepper
#2 2942068-2.patch1.6 KBkim.pepper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kim.pepper created an issue. See original summary.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Active » Needs review
FileSize
2942068-2.patch1.6 KB

Initial patch.

Status: Needs review » Needs work

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

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Needs review
FileSize
2942068-4.patch2.35 KB
2942068-4-interdiff.txt774 bytes

Updates the test to check for the number of error messages. There is actually 2.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented 
  1. +++ b/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php
@@ -143,9 +143,11 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+    $beforeMessages = drupal_get_messages('error', FALSE);
+    drupal_set_message($this->t('Number of error messages before _file_save_upload_from_form(): @count.', ['@count' => isset($beforeMessages['error']) ? count($beforeMessages['error']) : 0]));
...
+    $afterMessages = drupal_get_messages('error', FALSE);
+    drupal_set_message($this->t('Number of error messages after _file_save_upload_from_form(): @count.', ['@count' => isset($afterMessages['error']) ? count($afterMessages['error']) : 0]));

    I'd say we are already touching these lines of code. Can we remove deprecated function calls?

  2. +++ b/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php
+    drupal_set_message($this->t('Number of error messages before _file_save_upload_from_form(): @count.', ['@count' => isset($beforeMessages['error']) ? count($beforeMessages['error']) : 0]));
...
+    drupal_set_message($this->t('Number of error messages after _file_save_upload_from_form(): @count.', ['@count' => isset($afterMessages['error']) ? count($afterMessages['error']) : 0]));

    Isn't this a BC break if we have to check isset whereas in before messenger service we don't have to?

  3. +++ b/core/modules/file/tests/file_test/src/Form/FileTestSaveUploadFromForm.php
@@ -143,9 +143,11 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+    $beforeMessages = drupal_get_messages('error', FALSE);
...
+    $afterMessages = drupal_get_messages('error', FALSE);

    Can we somehow add default to these vars and avoid calling isset?

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
  1. Yep sure.
  2. drupal_get_messages() returns a nest array keyed by type. MessengerInterface::getMessagesByType() just returns the messages.
  3. Not sure what you mean?
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Issue tags: +messenger
FileSize
2942068-7.patch4.77 KB
2942068-7-interdiff.txt4.18 KB

Converted to MessengerInterface. This removes the need to check isset.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks, for addressing the feedback.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2942068-7.patch, failed testing. View results

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
CreditAttribution: kim.pepper commented
Status: Needs work » Reviewed & tested by the community

Rested so back to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 7: 2942068-7.patch, failed testing. View results

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs work » Reviewed & tested by the community

Unrelated Jenkins DB fails.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at Lemberg Solutions, Drupal Ukraine Community for Drupal Ukraine Community, Lemberg Solutions commented

+1 for RTBC

alexpott’s picture

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

Committed and pushed c10cc9ec84 to 8.6.x and a8802f74cd to 8.5.x. Thanks!

Backported as a test only change.

  • alexpott committed c10cc9e on 8.6.x 
    Issue #2942068 by kim.pepper, jibran: FileTestSaveUploadFromForm...

  • alexpott committed a8802f7 on 8.5.x 
    Issue #2942068 by kim.pepper, jibran: FileTestSaveUploadFromForm...

Status: Fixed » Closed (fixed)

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