In this code block, theme_status_messages() hardcodes a 0 index for the $messages array when theming a single value message array:

    if (count($messages) > 1) {
      $output .= " 
    \n"; foreach ($messages as $message) { $output .= '
  • ' . $message . "
  • \n"; } $output .= "
\n"; } else { $output .= $messages[0]; }

The problem is that if you're doing any sort of manual interaction with the messages session variable, you may end up with a single value array that does not have a 0 key. Instead of adding $messages[0] to $output, this could be changed to reset($messages) if desired. I'm sure there are other ways to deal with this, the most robust of which will not get into 7.x - provide a proper API around status messages. ^_^

Comments

rszrama’s picture

Status: Active » Needs review
FileSize
879 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,102 pass(es). View

Quick patch, though certainly not the only approach. It's worth noting that the function doesn't really care what the index is, it just made an unsafe assumption.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Works for me, doesn't need to go into 8.x because #1939082: Convert theme_status_messages() to Twig will remove that function altogether

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs backport to D7

If the code exists in Drupal 8, it needs to be committed to Drupal 8 first.

Also, if I'm reading #1939082: Convert theme_status_messages() to Twig correctly, won't the current patch in that issue simply wind up taking this bug and porting it over to the Twig template? :)

David_Rothstein’s picture

I wouldn't have thought of this except for the above... but could this use tests?

Maybe it's a bit of overkill to test this, but on the other hand, I certainly find myself altering/removing status messages all the time, so having a test to ensure that you can do that without the display breaking might be reasonable.

Beyond that, the patch certainly looks good.

rszrama’s picture

As far as I understood it, the method for Drupal 8 would either be a different Twig variable operator or else a different patch to the preprocess function to actually reindex the messages array. In either case, the D7 patch is irrelevant to D8 and I proposed the fix in #1939082-8: Convert theme_status_messages() to Twig.

I thought about tests as well but wasn't sure what you'd be testing... in this case, that reset() worked as expected?

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1946240.theme_status_messages_reset-6.patch. Unable to apply patch. See the log in the details link for more information. View
2.64 KB
FAILED: [[SimpleTest]]: [MySQL] 53,322 pass(es), 1 fail(s), and 1 exception(s). View

I was thinking of a functional test, something like this.

David_Rothstein’s picture

While writing this, though, I realized that the above test passes/fails as expected only because of this:

  // Remove the first.
  unset($_SESSION['messages']['status'][0]);

However, do we really support that?

If you instead do it using the API (which, granted, involves a lot less straightforward code):

  // Remove the first.
  $messages = drupal_get_messages();
  unset($messages['status'][0]);
  foreach ($messages['status'] as $message) {
    drupal_set_message($message);
  }

Then everything works and there is no bug.

But then again, this issue is filed as a task rather than a bug report :) And I think switching to reset() is a reasonable idea anyway.

jcisio’s picture

David_Rothstein’s picture

Ah, I see. But in the end, looking at http://api.drupalize.me/api/drupal/function/commerce_checkout_form_valid... it still seems to do something similar:

  // Restore messages and form errors.
  $_SESSION['messages'] = array_merge_recursive(array_filter($previous_messages), drupal_get_messages());

So ultimately you still only hit an issue here if you manipulate $_SESSION['messages'] directly, rather than calling drupal_set_message() to re-set the messages you want to keep around.

Question I guess is whether we want to "say it's OK" to manipulate $_SESSION['messages'] directly by asserting things about that behavior in a test.

ExTexan’s picture

How about backporting this patch to D6 and (I assume) D7? I had this problem in D6.

The last submitted patch, 6: 1946240.theme_status_messages_reset-6.patch, failed testing.

Cottser’s picture

Title: Remove the hardcode 0 index in theme_status_messages() » Remove the hardcode 0 index in status-messages.html.twig
Issue summary: View changes
Status: Needs review » Active
Issue tags: +Twig, +Novice

This is still relevant for 8.0.x. Looking back to #1939082-9: Convert theme_status_messages() to Twig, I think we can simply update the Twig template to use the "first" filter because we are now on Twig 1.15.0: http://twig.sensiolabs.org/doc/filters/first.html

Cottser’s picture

Title: Remove the hardcode 0 index in status-messages.html.twig » Remove the hardcoded 0 index in status-messages.html.twig
madhusudanmca’s picture

Status: Active » Needs review
FileSize
469 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,860 pass(es), 819 fail(s), and 4,686 exception(s). View

Just removed 0 index.

Status: Needs review » Needs work

The last submitted patch, 15: theme_status_messages_reset-1946240-15.patch, failed testing.

Yaron Tal’s picture

Status: Needs work » Needs review
FileSize
475 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,702 pass(es). View

Using {{ messages|first }} as suggested by Cottser.

Yaron Tal’s picture

Issue tags: +Amsterdam2014
eiriksm’s picture

Just tested this. Works as expected. Should we add a test (ref #6)?

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

For sure, I should have tagged it. Thanks @Yaron Tal and @eiriksm!

eiriksm’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,711 pass(es). View

With tests (sorry for hijacking the patching @Yaron Tal)

Tests are based on #6, but uses the new folder structure and so on...

Cottser’s picture

Cool, thanks! Can we get a test-only patch please?

eiriksm’s picture

FileSize
2.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,744 pass(es), 1 fail(s), and 0 exception(s). View

Yup, there we are.

Status: Needs review » Needs work

The last submitted patch, 23: drupal_set_message_TESTS_ONLY-1946240-23.patch, failed testing.

eiriksm’s picture

Status: Needs work » Needs review

Failed as expected. Setting back to needs review.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Great, just a couple small tweaks and I think this is good to go. Thanks @eiriksm!

  1. +++ b/core/modules/system/src/Tests/Bootstrap/DrupalSetMessageTest.php
    @@ -0,0 +1,37 @@
    + * Definition of Drupal\system\Tests\Bootstrap\DrupalSetMessageTest.
    

    The new standard here is Contains \Drupal\system….

    Per https://www.drupal.org/node/1354#file.

  2. +++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
    @@ -25,6 +25,21 @@ public function mainContentFallback() {
       /**
    +   * Tests setting messages and removing one before it is displayed.
    +   * @return string
    +   *   Empty string, we just test the setting of messages.
    +   */
    

    Need a line between the description line and the @return. https://www.drupal.org/node/1354#general

eiriksm’s picture

FileSize
3.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,024 pass(es). View

Ah, that is all true.

New patch attached.

eiriksm’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: drupal_set_message_with_tests-1946240-27.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

alexpott’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 1d70740 and pushed to 8.0.x. Thanks!

  • alexpott committed 1d70740 on 8.0.x
    Issue #1946240 by eiriksm, David_Rothstein, Yaron Tal, madhusudanmca,...
er.pushpinderrana’s picture

Status: Patch (to be ported) » Needs review
FileSize
357 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,910 pass(es). View

Patch for D7.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Great, thanks @er.pushpinderrana! This needs tests for D7 now, I think there were some earlier in the thread (or we can backport the test that was committed to 8.0.x).

ryanissamson’s picture

Assigned: Unassigned » ryanissamson

I'll pick this one up. :)

dgtlife’s picture

Assigned: ryanissamson » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.91 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

I combined the respective patches for drupal_set_message() and the associated test functions in Drupal 7

Status: Needs review » Needs work

The last submitted patch, 39: remove_the_hardcoded_0-1946240-39.patch, failed testing.

hampercm’s picture

Assigned: Unassigned » hampercm

Fixing test in patch from #39

hampercm’s picture

Status: Needs work » Needs review
Issue tags: -needs backport to D7
FileSize
2.89 KB
PASSED: [[SimpleTest]]: [MySQL] 41,134 pass(es). View
1.74 KB

Corrected the backported test contained in #39 to function with D7 SimpleTest. Also fixed 4 whitespace issues contained in the patch from #39.

Cottser’s picture

Awesome, thank you both @dgtlife and @hampercm! I think all we need now is a test-only D7 patch uploaded to ensure the test will catch a regression.

hampercm’s picture

Assigned: hampercm » Unassigned
hampercm’s picture

FileSize
2.54 KB
FAILED: [[SimpleTest]]: [MySQL] 41,132 pass(es), 1 fail(s), and 1 exception(s). View

Here is the test-related portion split out from the patch in #42. Is this what you were looking for, @Cottser ?

Status: Needs review » Needs work

The last submitted patch, 45: remove_the_hardcoded_0-1946240-test_only-45.patch, failed testing.

Cottser’s picture

That's great, thank you @hampercm! Just a few small points but otherwise this looks RTBC to me.

  1. +++ b/modules/simpletest/tests/system_test.module
    @@ -420,3 +427,18 @@ function system_test_authorize_init_page($page_title) {
    +function system_test_drupal_set_message() {
    +
    +  // Set two messages.
    

    Minor: We can remove the blank line here.

  2. +++ b/modules/system/system.test
    @@ -2825,3 +2825,35 @@ class SystemValidTokenTest extends DrupalUnitTestCase {
    +      'group' => 'Bootstrap',
    

    It looks like a group of 'System' would be more appropriate, all the 'Bootstrap' tests are in bootstrap.test.

  3. +++ b/modules/system/system.test
    @@ -2825,3 +2825,35 @@ class SystemValidTokenTest extends DrupalUnitTestCase {
    +
    +    $this->admin_user = $this->drupalCreateUser(array('access administration pages', 'administer modules'));
    +    $this->drupalLogin($this->admin_user);
    

    Are these lines in the test needed? Seems like they shouldn't be for testing this functionality.

hampercm’s picture

Status: Needs work » Needs review
FileSize
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] 41,122 pass(es), 1 fail(s), and 1 exception(s). View
2.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch remove_the_hardcoded_0-1946240-48.patch. Unable to apply patch. See the log in the details link for more information. View
1.06 KB

Thank you @Cottser. Here is the updated patch and test-only patch.

The last submitted patch, 48: remove_the_hardcoded_0-1946240-test_only-48.patch, failed testing.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, thanks again @hampercm. Tests look good and from my perspective this meets the core gates. RTBC!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: remove_the_hardcoded_0-1946240-48.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 48: remove_the_hardcoded_0-1946240-48.patch, failed testing.

Cottser’s picture

Title: Remove the hardcoded 0 index in status-messages.html.twig » Remove the hardcoded 0 index in theme_status_messages()
Issue tags: +Needs reroll

Better title for D7 commit, and this needs reroll.

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.41 KB
FAILED: [[SimpleTest]]: [MySQL] 41,177 pass(es), 1 fail(s), and 1 exception(s). View
2.76 KB
PASSED: [[SimpleTest]]: [MySQL] 41,376 pass(es). View

The last submitted patch, 56: 1946240-56-test_only.patch, failed testing.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @rpayanm!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

  • David_Rothstein committed 6306562 on 7.x
    Issue #1946240 by hampercm, eiriksm, David_Rothstein, rpayanm, rszrama,...

Status: Fixed » Closed (fixed)

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