Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#6112 closed defect (bug) (fixed)

Template notices html

Reported by: slaFFik Owned by: imath
Milestone: 2.2 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch commit
Cc:

Description

We are an ordinary user, on Settings page.

To change the password we input incorrect current one, new and repeat it.
As the password is incorrect we get such an error: http://screencast.com/t/lZQtmvECC3t

Please look at the no space between sentences. If you look at the html source code - everything will be in 1 <p> tag.
If you look at the php source code around No changes were made to your account message, you will see that error messages are imploded using </p><p>. If you look at bp_core_render_message() you will see that there is no <p> tag around echo $content.

So we have html that is generated by BP:

<div id="message" class="bp-template-notice error">
    Your current password is invalid.</p><p>No changes were made to your account.
</div>

And the browser transforms it into:

<div id="message" class="bp-template-notice error">
    <p>Your current password is invalid.No changes were made to your account.</p>
</div>

So the styling is wrong.

Replicated on both BuddyPress 2.1.1 and 2.2 beta 1

Attachments (2)

6112-remove-kses.diff (579 bytes) - added by Mamaduka 5 years ago.
6112.02.patch (493 bytes) - added by imath 5 years ago.

Download all attachments as: .zip

Change History (17)

#1 @slaFFik
5 years ago

  • Component changed from Settings to Core

#2 @slaFFik
5 years ago

See also #6030

#3 @DJPaul
5 years ago

  • Milestone changed from Awaiting Review to 2.2

Let's sneak a fix in for 2.2, as it ought to be pretty small.

#4 @Mamaduka
5 years ago

We already have wpautop for bp_core_render_message_content. Which should handle paragraphs. Issue is caused later in bp_core_render_message_content filters chain by wp_kses_data() which strips all HTML tags. We need to remove it or specify context.

#5 @Mamaduka
5 years ago

  • Keywords reporter-feedback added

#6 @imath
5 years ago

  • Keywords has-patch added; reporter-feedback removed

Hi,

I think the easiest is to avoid using html tags to join the feedback messages and leave the filters the way they are. See 6112.02.patch.

@Mamaduka, i think we need to sanitize the output and removing the filter 'wp_kses_data' can be a trouble from my point of view.

Last edited 5 years ago by imath (previous) (diff)

@imath
5 years ago

#7 @Mamaduka
5 years ago

@imath

I don't like removing wp_kses_data as well, but it will strip all HTML. Do we have any internal kses wrapped with minimum allowed HTML?

#8 @imath
5 years ago

  • Keywords reporter-feedback added

What is wrong with 6112.02.patch ?

Is this the fact the feedback messages are only separated by a line feed and not "encapsulated" in a <p> tag ?

If so then, i prefer to have a specific kses function that will only allow the minimum tags. Best would be to filter 'wp_kses_allowed_html' and build some BuddyPress kses contexts (activity, messages, notices...)

But i think just using 6112.02.patch might be enough ? What do you think slaFFik and Mamaduka ?

#9 @slaFFik
5 years ago

  • Keywords dev-feedback added; reporter-feedback removed

@imath
You patch is the easiest, but I like @Mamaduka's more, as it will give ability to use html in template notices.
There is nothing that will be added in those notices by non-devs. And devs might want to have more control - even in using html there.

#10 @slaFFik
5 years ago

I'm talking about tooltips, for example.

#11 @imath
5 years ago

i don't know why but it seems very weird to me to print some html tags in cookies.. But at the very least, i really think it's safer we sanitize this output.

#12 @r-a-y
5 years ago

i don't know why but it seems very weird to me to print some html tags in cookies.

I kind of agree. Maybe we need to stop using cookies to save and display temporary messages. We probably should be using usermeta or PHP sessions.


I definitely do not think we should remove KSES as the patch in 6112-remove-kses.diff suggests. Especially for something that we're relying on a cookie for content.

imath has the right idea:

If so then, i prefer to have a specific kses function that will only allow the minimum tags. Best would be to filter 'wp_kses_allowed_html' and build some BuddyPress kses contexts (activity, messages, notices...)


However, for this specific problem on the Settings page, the existing implode() code is ugly, but imath's patch addresses the "no space" issue between multiple messages. I say we commit that for now and circle back to refactoring bp_core_render_message() to allow HTML for 2.3.

Last edited 5 years ago by r-a-y (previous) (diff)

#13 @Mamaduka
5 years ago

Sorry for confusion, I only submitted patch to highlight where issue was caused.

I think it would be better, if we got with @imath's patch for this ticket and open new one for next major release.

r-a-y, I don't think using PHP session is best solution here. Since some managed WP host disable those (e.g. WPE). We definitely need to move whole impode/HTML part into render functions. Maybe do something like WP's *_setting_error() functions do.

#14 @imath
5 years ago

  • Keywords commit added; dev-feedback removed

no problem Mamaduka, i'll commit the 6112.02.patch tonight and as jjj/r-a-y and you suggested, we could work on a BP_User_Feedback class to extend WP_Error in a near future

#15 @imath
5 years ago

  • Owner set to imath
  • Resolution set to fixed
  • Status changed from new to closed

In 9355:

Better separation of template notices on the user settings page

In case of more than one feedback messages are added to template notices of the user settings page, make sure the separation between each of them are clear enough.

Fixes #6112

Note: See TracTickets for help on using tickets.