WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 21 months ago

#23012 closed enhancement (fixed)

Refresh the code for the default widgets

Reported by: Viper007Bond Owned by: wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.5
Component: Widgets Keywords:
Focuses: Cc:

Description (last modified by westonruter)

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.

Attachments (8)

23012.diff (63.4 KB) - added by welcher 2 years ago.
First pass of default-widgets.php
23012.2.diff (56.8 KB) - added by welcher 2 years ago.
Patch refresh and items mentioned by @jdgrimes
23012.3.diff (62.7 KB) - added by welcher 23 months ago.
Patch refresh and removing esc_html on strings
23012.4.diff (66.7 KB) - added by welcher 22 months ago.
Post 4.3 patch refresh
23012.5.diff (36.3 KB) - added by wonderboymusic 21 months ago.
23012.6.diff (33.3 KB) - added by wonderboymusic 21 months ago.
23012.7.diff (31.9 KB) - added by wonderboymusic 21 months ago.
23012.8.diff (20.1 KB) - added by wonderboymusic 21 months ago.

Download all attachments as: .zip

Change History (51)

#1 @Viper007Bond
4 years ago

  • Owner set to Viper007Bond
  • Status changed from new to accepted

#2 follow-up: @toscho
4 years ago

  • Cc info@… added

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

#3 in reply to: ↑ 2 @Viper007Bond
4 years ago

  • Milestone changed from Awaiting Review to Future Release

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?

#4 @toscho
4 years ago

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.

#5 @bradyvercher
4 years ago

  • Cc brady@… added

#6 @Viper007Bond
4 years ago

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.

#7 @chriscct7
2 years ago

  • Keywords good-first-bug added
  • Severity changed from minor to normal

This would make a great first bug patch for someone

#8 @Viper007Bond
2 years ago

  • Owner Viper007Bond deleted
  • Status changed from accepted to assigned

@welcher
2 years ago

First pass of default-widgets.php

#9 @welcher
2 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

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.

#10 @chriscct7
2 years ago

  • Owner set to chriscct7
  • Priority changed from low to normal
  • Status changed from assigned to reviewing

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?

#11 @chriscct7
2 years ago

  • Keywords dev-feedback removed

#12 @welcher
2 years ago

@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.

#13 @chriscct7
2 years ago

@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

#15 follow-up: @chriscct7
2 years ago

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 :-)

#16 in reply to: ↑ 15 @welcher
2 years ago

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!

#17 @jdgrimes
2 years ago

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
2 years ago

Patch refresh and items mentioned by @jdgrimes

#18 @welcher
2 years ago

  • Keywords dev-feedback added

#19 follow-ups: @chriscct7
2 years ago

@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

#20 in reply to: ↑ 19 @jdgrimes
2 years ago

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).

#21 in reply to: ↑ 19 ; follow-up: @welcher
2 years ago

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.

#22 in reply to: ↑ 21 ; follow-up: @ocean90
2 years ago

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.

#23 in reply to: ↑ 22 @toscho
2 years ago

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

#24 follow-up: @chriscct7
23 months ago

  • Keywords needs-refresh added; dev-feedback removed

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
23 months ago

Patch refresh and removing esc_html on strings

#25 in reply to: ↑ 24 @welcher
23 months ago

  • Keywords dev-feedback added; needs-refresh removed

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.

#26 @chriscct7
23 months ago

  • Keywords dev-feedback removed
  • Status changed from reviewing to accepted

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

This ticket was mentioned in Slack in #core by welcher. View the logs.


23 months ago

#28 @welcher
22 months ago

  • Keywords early added

#29 @welcher
22 months ago

  • Milestone changed from Future Release to 4.4

#30 @chriscct7
22 months ago

  • Keywords dev-feedback needs-patch added; good-first-bug has-patch early removed

Some quick feedback

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

#31 @westonruter
22 months ago

  • Description modified (diff)

#32 @welcher
22 months ago

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

#33 follow-up: @jorbin
22 months ago

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
22 months ago

Post 4.3 patch refresh

#34 @welcher
22 months ago

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

#35 in reply to: ↑ 33 @welcher
22 months ago

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?

#36 @wonderboymusic
21 months ago

In 33813:

Improve/update escaping in WP_Widget_Pages.

Props welcher.
See #23012.

#37 follow-up: @wonderboymusic
21 months ago

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.

#38 in reply to: ↑ 37 @GaryJ
21 months ago

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.

#39 @wonderboymusic
21 months ago

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.

#41 @wonderboymusic
21 months ago

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.

#42 @wonderboymusic
21 months ago

  • Keywords dev-feedback needs-patch removed
  • Owner changed from chriscct7 to wonderboymusic
  • Status changed from accepted to assigned

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

#43 @wonderboymusic
21 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.