search

studiopress / agentpress-listings

AgentPress Listings
http://wordpress.org/plugins/agentpress-listings/
GNU General Public License v2.0
9 stars 11 forks source link

Fix video and map textarea #40

Closed marksabbath closed 5 years ago

marksabbath commented 5 years ago

This PR fixes the bug that affects the map and video embed textareas. We have updated the escaping function, which now is using wp_kses with some HTML tags allowed by default. Also, we've added a new filter agentpress_featured_listings_allowed_html where users could change these allowed tags.

Closes #39

marksabbath commented 5 years ago

Wouldn't be better to use wp_strip_all_tags for those @nickcernis ? I guess that those input shouldn't show any HTML tag, and our $allowed_tags actually are allowing HTML tags.

@marksabbath This is working for me for the map and video fields — nice work! (I made a comment about the short array syntax separately.)

The filter is also working for me. I tested it with this:

add_filter( 'agentpress_featured_listings_allowed_html', function( $allowed_html ) {
  $allowed_html['span'] = array();
  return $allowed_html;
});

One thing I noticed is that we're allowing some HTML in the input fields, but we then use esc_html() when outputting those fields on the front end in the property_details_shortcode( $atts ) function, so the HTML displays on the front end:

Screen Shot 2019-05-31 at 13 54 04 Screen Shot 2019-05-31 at 13 54 28

Perhaps we should use wp_kses with our $allowed_tags for the post meta we output there too (the labels could continue to use esc_html). What do you think?

https://github.com/studiopress/agentpress-listings/blob/38a47dcaaec613da58ca1d5188991ef6f4b5d8a9/includes/class-agentpress-listings.php#L275-L283

nickcernis commented 5 years ago

Wouldn't be better to use wp_strip_all_tags for those @nickcernis ? I guess that those input shouldn't show any HTML tag, and our $allowed_tags actually are allowing HTML tags.

I entered those <p> tags manually, so I think they should show in that case. Does that make sense?

marksabbath commented 5 years ago

Oh, I'm sorry, I totally made a confusion here! Yeap! Totally makes sense man!

I'll make those updates right away!

Wouldn't be better to use wp_strip_all_tags for those @nickcernis ? I guess that those input shouldn't show any HTML tag, and our $allowed_tags actually are allowing HTML tags.

I entered those <p> tags manually, so I think they should show in that case. Does that make sense?