Refresh the code for the default widgets

[Viper007Bond]

The default widgets serve as examples for many people to make new widgets. We should make sure they're up to date in terms of coding standards and best practices. There's many places where this is not the case.

A few examples:

• $instance['text'] = stripslashes( wp_filter_post_kses( addslashes($new_instance['text']) ) );
• $text = esc_textarea($instance['text']); rather than at output (implement late-escaping of echo'ed variables)

Also tons and tons of formatting issues.

I'll volunteer to take this on.

[toscho]

I would like to help. The class methods should be atomic: one method for each step to make these widgets reusable.

[Viper007Bond]

Replying to toscho:

The class methods should be atomic: one method for each step to make these widgets reusable.

Sorry, I don't understand. Can you explain, or perhaps give an example?

[toscho]

Let’s say I want to extend WP_Widget_Meta to add or replace a link in front-end. Currently I have to copy almost the complete class to add these simple change. If the ul for the link were generated in a separate method in that class I could replace just this method.

Or try to replace the Feed icon in WP_Widget_RSS. This should be a separate method get_feed_icon().

The same applies to the other classes: everything is mangled together with very little room for sub-classes.

[Viper007Bond]

Ah, so you just want more abstraction so it's easier to to replace specific pieces of the widget.

That's a valid suggestion but it also makes it harder to understand them and less useful as examples. However extensibility may be more important than simply serving as examples. Perhaps documentation or even a sample widget at the top of the file in a comment would be a better place for an example widget.

[chriscct7]

This would make a great first bug patch for someone

[welcher]

First pass of default-widgets.php

[welcher]

First pass patch to address code style and to escape all of the things. Definite +1 to having more abstraction in the classes but wanted to get this pass done first.

[chriscct7]

Going to review this. I think what we'll do here is look at reviewing and merging the code issues patch you made for 4.3, and then I think we should open a new ticket for abstractions, as we really should get many oppinions on that before proceeding so we can ensure we don't tunnel vision on that. @welcher did you want to take a run at abstractions for 4.4 or did you want someone else to work on that?

[welcher]

@chriscct7 I would love to take a run at abstractions. Did you want me to open the abstractions ticket? In the meantime, if there is anything in the patch that I missed or you disagree with, please let me know.

[chriscct7]

@welcher, go ahead an open a new one as enhancement and then write as much as you can (detailed wise) how you would abstract the widget. Make sure to put a link as related to: #(this ticket number) and then maybe tag in dev-feedback, so we can get a discussion going on that. I'll review this sometime this weekend probably

[welcher]

Related #32470

[chriscct7]

Thanks @welcher! We'll make sure to get this reviewed. I see your new ticket has started an excellent discussion. Nice job! Keep up the good work + thanks for contributing :-)

[welcher]

Replying to chriscct7:

Thanks @welcher! We'll make sure to get this reviewed. I see your new ticket has started an excellent discussion. Nice job! Keep up the good work + thanks for contributing :-)

Anytime! I'm really excited to be able to give back to the platform that pays my bills :)

I just wanted to followup to see if there is anything I need to do or update for the current patch. I'm happy to make any changes/additions needed.

Thanks!

[jdgrimes]

On lines 424-5 wp_kses() is used on strings that are meant to be translated, but the __() function is omitted.

Spacing issues with a couple of the periods on line 1046:

$title = '<a class="rsswidget" href="' . esc_url( $url ). '"><img style="border:0" width="14" height="14" src="'. esc_url( $icon ) . '" alt="RSS" /></a> <a class="rsswidget" href="' .esc_url( $link ) .'">' . $title . '</a>'; 

Also, there should probably be a newline at the end of the file.

[welcher]

Patch refresh and items mentioned by @jdgrimes

[chriscct7]

@welcher The changes like esc_html_e( 'Page IDs, separated by commas.' ); where the string is hardcoded don't need to be escaped (because they are hardcoded). Running esc_html on those just is a little performance hit for no real gain there

[jdgrimes]

Replying to chriscct7:

@welcher The changes like esc_html_e( 'Page IDs, separated by commas.' ); where the string is hardcoded don't need to be escaped (because they are hardcoded). Running esc_html on those just is a little performance hit for no real gain there

​Some would disagree (myself included).

[welcher]

Replying to chriscct7:

@welcher The changes like esc_html_e( 'Page IDs, separated by commas.' ); where the string is hardcoded don't need to be escaped (because they are hardcoded). Running esc_html on those just is a little performance hit for no real gain there

Anything that is being translated is potentially an attack vector if the output is not escaped. It's not the hardcoded string in the method that is the concern but rather the translated string. It is common practice to escape translated output when working with WordPress.com VIP and while I realize this is not VIP, I think having these in-place provided a good example for those that may be using these widgets as a starting point. I personally think the small amount of performance hit is worth the security gain.

[ocean90]

Replying to welcher:

It is common practice to escape translated output when working with WordPress.com VIP […]

That's true, but in WordPress core we trust translations, see recent discussion in #30724.

[toscho]

Replying to ocean90:

That's true, but in WordPress core we trust translations, see recent discussion in #30724.

That’s a very, very bad idea. WP has no control over the final location of the language files and their content. A simple symlink placed by an attacker on a self-hosted installation can replace the file already. See ​http://wordpress.stackexchange.com/a/138677/73

[chriscct7]

After a discussion with @obenland and a couple others on the topic, the group consensus is we need to move forward without the escaping on translations.

@welcher can you refresh the patch as such so we can get it in?

[welcher]

Patch refresh and removing esc_html on strings

[welcher]

Replying to chriscct7:

After a discussion with @obenland and a couple others on the topic, the group consensus is we need to move forward without the escaping on translations.

I would love to continue this conversation in the future but lets not let it bog us down so this ticket doesn't move forward.

@welcher can you refresh the patch as such so we can get it in?

Patch refreshed - please let me know if there is anything I have missed.

[chriscct7]

Looks good. We might have to tag this for 4.4. @obenland?

[chriscct7]

Some quick feedback

if ( 'menu_order' == $sortby ) { should be if ( 'menu_order' === $sortby ) { since we're doing a string compare

[welcher]

Patch doesn't apply cleanly for me. I'll refresh.

[jorbin]

I don't like all the unnecessary formatting changes here. They will make debugging via blame harder. Yes, they may not be up to the current code standards, but churning for the sake of code style makes development harder in the long run.

[welcher]

Post 4.3 patch refresh

[welcher]

New patch attached refreshed for 4.3. Should also address #33235.

[welcher]

Replying to jorbin:

I don't like all the unnecessary formatting changes here. They will make debugging via blame harder. Yes, they may not be up to the current code standards, but churning for the sake of code style makes development harder in the long run.

I definitely see your point - perhaps rolling them into #33413 might make sense?

[wonderboymusic]

In 33813:

Improve/update escaping in WP_Widget_Pages.

Props welcher.
See #23012.

[wonderboymusic]

In 33814:

Improve/update escaping in default widgets:

• wrap some variables in esc_attr() before echoing
• replace some strip_tags() calls with sanitize_text_field()
• call esc_url() when wrapping some URLs

Props welcher.
See #23012.

[GaryJ]

Replying to wonderboymusic:

In 33814:

Each of the new <?php checked(...); ?> calls can have the space before the <?php removed, as checked() already outputs a leading space. There are at least two selected() calls that can have the equivalent space removed too.

[wonderboymusic]

In 33843:

Move widget classes to their own files in wp-includes/widgets:

• default-widgets.php now requires all of the individual classes
• Move the functions scattered about this file to widget-functions.php, which loads before default-widgets.php, which only conditionally loads anyway in wp_maybe_load_widgets(), which is hooked on plugins_loaded

See #33413, #23012.

[wonderboymusic]

In 33845:

De-Jorbin [33814].

See #23012.

[wonderboymusic]

In 33951:

After [33814], checked() outputs a leading space, so some extra spaces can be removed in the HTML for default widget forms.

See #23012.

[wonderboymusic]

Calling this done for now - I think calling esc_attr() on ->get_field_id() and ->get_field_name() is somewhat overkill.