User feedbacks : Extend WP_Error and progressively stop using cookies to store messages.

[imath]

We've been talking about this in slack, in this ticket #6112, and this ticket #6430 is another example of how we could take benefit of using a new class to extend WP_Error and manage the template notices (or feedback messages) we provide to users.

I've begun building a patch about this, but before going "fast & furious" about it :) I'd like to have your opinion on the directions i've taken.

• BP_Feecback extends WP_Error
• passing the message code (or a comma separated list of message codes) into the url
• setting the message codes in each component's class

see 01.patch

Here's some examples of it for :

• #6112

• #6430

There will be some complexity to deal with when a feedback message is dynamically created (e.g. the star feature bulk messages, the upload errors..)

[boonebgorges]

Our current cookie-based implementation stinks, and it would be nice to have the ability to show more than one message on a pageload.

That being said, the suggested improvement seems overengineered somehow. I have a couple of concerns and questions:

• Interfering in the redirect (add_query_arg()) seems like it's going to cause problems. What if we have a query arg clash? What will this do to caching? What about people who are modifying redirects in various ways?
• What's the advantage of extending WP_Error to do this?
• Are we going to have a recommended method for plugins to register their own feedback_messages? Do they have to have their own component to do so? What if, say, a messages attachment plugin wants to add items to messages->feedback_messages?
• Keying error messages by integers increases mental overhead during development and debugging. Can we use strings? (I guess this doesn't translate well? Then again, ?error=2,6 is not exactly a pretty URL in any language.) If you're going to use numeric error codes, at least you can use these codes as the array keys for the feedback_messages array ( 3 => 'foo' rather than array( 'code' => 3, 'message' => 'foo' )
• Abstracting the feedback_messages makes a bit more sense if any of them are shared between different error conditions. Is this the case?

I feel like there's gotta be a way to solve the core problem without introducing such a large amount of overhead. How about, eg, storing errors in usermeta? (Not sure what that would mean for logged-out users.) What about URL-encoding the error message itself?

[imath]

First, thanks a lot for your feedback.

Feedback messages have a time of persistance limited to next page load (max). I wanted to avoid writing data into the DB, that's why i've used the url to pass feedback codes.
Using user_meta seems the wrong moves to me, because as soon as we will delete the feedback message it will delete the whole 'user_meta' cache.
URL-encoding the message will result in very unpretty urls :)

But about the pretty urls concern, i'm not sure for this particular case it's a concern we should have, i think the real benefit of using pretty links is when you need to share them. I doubt people will be likely to share errors :)

WP_Error: i think jjj suggested to extend it for this particular need. It's an interesting way of structuring a feedback > code, message and extra data. I need to extend it because you can't get a list of particular messages: you can get all, or one. This explains why the feedback messages are structured like array( 'code' => 3, 'message' => 'foo' ) and not like array ( 3 => 'foo' ).

What if, say, a messages attachment plugin wants to add items to messages->feedback_messages?

You're right this needs to be taken in account, and as the patch is a very first draft, it's not dealing with this possibility, i was just testing my idea :) I guess we can create a new method in BP_Component a bit like the setup_admin_bar() function works.

I think (but i may be wrong) we need a standardized way of getting the message at runtime, and i think referencing errors using codes (numbers) is very common in software development (e.g.: 404). Moreover i can imagine the interest of these codes in unit tests :) I'm not sure using strings will be a great thing. About your concern on debugging: most of the time functions are returning false in case of an error with no extra informations, and i'm not changing anything to this. Are we using the $bp->template_message global for debug needs ? If so we can still get the error code global and fetch the message to set $bp->template_message with it if WP_DEBUG is on :)

You're probably right about the query arguments potential clash. If we don't want to change anything to bp_core_redirect() to avoid this risk, is using keyed transients, a bad idea ? eg: 'bp-' . bp_loggedin_user_id() . '-feedback' or 'bp-' . sanitize_key( bp_core_current_user_ip() ) . '-feedback' if the user is not logged in.

The easy move would be to directly store the message in it and forget about the WP_Error thing..., so of course we can just do that.

[DJPaul]

This ticket description is missing details as to why using cookies are a bad idea, and what the current shortcomings of using them are.

Feedback messages have a time of persistance limited to next page load (max).

Careful, don't make assumptions! They don't today, but could in future.

[imath]

I don't think using cookies is a bad idea when we use it to store tiny informations such as the activity scope or any loops oldest page..., more globally user browsing preferences. I just find it weird to store some text that can be pretty long to remove it at the next page load :) But if everybody's fine with it, feel free to mark this ticket as invalid :)

[boonebgorges]

Using cookies for feedback messages means a couple of things:

• There are weird character encoding issues. I can't find the tickets at the moment, but a number of times in the past we've had double-escaped characters, etc.
• The cookie will be loaded and messages displayed on the very next successful pageload. But this is not always the right place to show the message. Say, for example, you do something in one tab, but then load BP in another tab; the message will be shown on the second tab rather than the first. This probably doesn't happen much in regular use, but I see it fairly often during development (especially when errors etc result in broken redirects and pageloads).

All this being said, the cookie implementation works fine, so I don't think it's worth introducing a large infrastructure to replace it.

After hearing more about why you chose WP_Error to build this proof-of-concept, I feel pretty sure that it's not the best idea. For one thing, many of our messages are not errors at all :) It also seems like an abuse somehow: WP_Error objects are meant to be developer-facing, and it's likely that developers and site admins have developed workflows for logging WP_Error-formatted errors. Using the class for our purposes could interfere.

Despite what I suggested earlier, I think that a transient/usermeta is probably not much of an improvement on the cookie implementation, and it adds the overhead of going to the database (and interfering with caching, as you note).

If there's a situation where the current cookie implementation is causing specific problems, let's address them. Otherwise I guess I lean toward closing this ticket.

[imath]

All this being said, the cookie implementation works fine, so I don't think it's worth introducing a large infrastructure to replace it.

Fine with me.