Default drupal_set_message $repeat to FALSE instead of TRUE (followup)

drupal_set_message_repeat.patch queued for re-testing.

I like this. I found this looking for an issue about repeated messages with update.module (as below). I think that issue could be resolved here.

There are security updates available for one or more of your modules or themes. To ensure the security of your server, you should update immediately! See the available updates page for more information and to install your missing updates.

There are security updates available for one or more of your modules or themes. To ensure the security of your server, you should update immediately! See the available updates page for more information and to install your missing updates.

I somehow had in mind that the current default was for performance reasons. That's totally bogus, though. The difference is a single in_array() and since the function is not called that often, that is definitely negligible.

I totally like this patch, I guess it could use a couple more approvals though before RTBC. Code-wise it's perfectly fine. The change to _drupal_log_error() totally makes sense.

Makes sense to me.

would it be helpful to know how many times a message was generated though for debugging purposes? I do find the current "show everything" policy annoying (especially with local development and php notices active) but knowing an error / message was generated multiple times could help debug faulty logic. Maybe tie it to a global defined value for the default so that other projects (like devel) could optionally switch it to default the other direction?

_drupal_log_error() explicitly sets $repeat to to TRUE, so for errors we still get that info.

Looks good. +1

Let's go back to RTBC.

This looks like a better default to me to. Committed to 8.x. Thanks.

I think we should backport it.

The backport is sound.
Marking this RTBC to get it in David Rothstein's queue.
Usually we don't backport these types of changes to D7, but in this case, I cannot imagine that anyone reasonably *expects* the same error/message to appear multiple times. Hmm...

I can definitely think of other situations where you expect the message to repeat.

For example, what about a very long form with more than one field that has the same name (maybe imagine two "address" or "phone number" fields that are part of two different field collections)? If there are errors in both, then it helps the user to see two separate error messages to indicate that, even if the text is the same.

I'm not sure if Drupal 8 wants to change the form API to deal with that (the above patch didn't, but reopen this if you think it should)...

But for Drupal 7, the fact that we already have a couple good use cases for the current behavior mentioned in this issue means (to me, at least) that there's really no good reason to change the default out from under people. The parameter exists and is well-documented; for anyone who wants to prevent repeated messages in their code, it's very simple to do that :)

Ok.

I entered that issue because of #1817680: Update notification is printed more than one time. For me at least, it was annoying to wait 3 days to upgrade to Drupal 7.16, seeing multiple warnings in every admin page. But yes, preserving some behaviors is probably important.

I'm not sure if Drupal 8 wants to change the form API to deal with that (the above patch didn't, but reopen this if you think it should)...

I now think it should :)

And honestly think we should consider rolling this whole patch back? The primary effect of this change really seems to be that it hides bugs in the calling code. (If your drupal_set_message() is being called more than once on the same page request, there's a very very good chance it's because code that you don't expect to run more than once is erroneously running more than once anyway. And in the rare event where you really do expect it to run more than once, passing $repeat as FALSE is easy, but at least we force you to explicitly acknowledge that you expect that.)

From what I've observed, usually it's not the same page requests (in d6 at least) -- hook_init + drupal_goto() + hook_init again.

Hm, that is probably true.

I agree with David_Rothstein in that this potentially hides bugs, and am thankful this hasn't (yet?) been backported to Drupal 7.

I also agree that message flooding for the user is a poor experience, but what about a compromise? Playing off of btopro's comment in #10, instead of silently dropping the additional error messages, note the number of repeats in the error message. Something like:

The foo has an error (3 additional messages hidden).

This mimics how Safari/Chrome handle multiply-repeating messages in the console (they stick a grey number next to the error):

So from reading through a solution appears to have already been committed to D8 core; perhaps this can be reopened under a different issue if people are passionate enough about an additional change but that way this can be marked done and get something out of the D8 issue queue that's already been resolved (as per original request / design decision) :)