Remove the hardcoded 0 index in theme_status_messages()

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

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? :)

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.

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

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.

#7 David, this issue came from #1945758: Work around theme_status_messages() limitation to display checkout form errors outside panes.

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.

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

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

Just removed 0 index.

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

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

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

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

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

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

Yup, there we are.

Failed as expected. Setting back to needs review.

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

Ah, that is all true.

New patch attached.

Looks good!

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

Patch for D7.

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

I'll pick this one up. :)

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

Fixing test in patch from #39

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

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.

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

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.

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

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

Better title for D7 commit, and this needs reroll.

Thanks @rpayanm!

Committed to 7.x - thanks!