Make WordPress Core

Opened 20 months ago

Last modified 6 weeks ago

#58102 new defect (bug)

Check PHPcs Coding standard into the wp-includes folder

Reported by: viralsampat's profile viralsampat Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords:
Focuses: coding-standards Cc:

Description

Hello Team,

I have reviewed the code and found a few errors in some of the below files. Please below files:

Files:

wp-includes/admin-bar.php
wp-includes/option.php

Thanks,

Attachments (13)

58102.diff (627 bytes) - added by viralsampat 20 months ago.
I have checked above mentioned issue and I have resolved it and added patch.
58102.2.diff (1.5 KB) - added by viralsampat 20 months ago.
I have checked above mentioned issue and I have resolved it and added patch.
58102.3.diff (1.3 KB) - added by viralsampat 20 months ago.
I have found one another file and added patch for the same.
58102.4.diff (1.3 KB) - added by dhrumilk 20 months ago.
@audrasjb Please Verify the diff it OK or does need to change.
58102-general-template.diff (1.3 KB) - added by viralsampat 19 months ago.
58102-template.diff (1.2 KB) - added by viralsampat 19 months ago.
I have removed unused variables from above mentioned files and checked phpcs coding standard.
58102-twentyseventeen-404.diff (942 bytes) - added by viralsampat 19 months ago.
I have checked above mentioned issue into the theme section and I have resolved it and added patch.
58102-twentysixteen.diff (5.7 KB) - added by viralsampat 18 months ago.
I have checked another file and added my patch
58102-twentyfifteen.diff (4.6 KB) - added by viralsampat 18 months ago.
I have checked another theme and founds few file. Here, I have added its patch.
58102-twentyfourteen.diff (9.1 KB) - added by viralsampat 18 months ago.
I have checked another theme and founds few file. Here, I have added its patch.
58102-twentythirteen.diff (5.1 KB) - added by viralsampat 18 months ago.
I have checked another theme and founds few file. Here, I have added its patch.
58102-twentytwelve.diff (15.1 KB) - added by viralsampat 18 months ago.
I have checked another theme and founds few file. Here, I have added its patch.
58102-twentyeleven.diff (24.2 KB) - added by viralsampat 18 months ago.
I have checked above mentioned issue and founds few files. Here, I have added its patch.

Download all attachments as: .zip

Change History (20)

@viralsampat
20 months ago

I have checked above mentioned issue and I have resolved it and added patch.

@viralsampat
20 months ago

I have checked above mentioned issue and I have resolved it and added patch.

#1 @SergeyBiryukov
20 months ago

  • Component changed from External Libraries to General

@viralsampat
20 months ago

I have found one another file and added patch for the same.

#2 @audrasjb
20 months ago

There is a small issue in your change on revison.php: __() should not be replaced with esc_html_e() but rather with esc_html__().

#3 @audrasjb
20 months ago

Also adding esc_attr() to $type_attr in
<style<?php echo $type_attr; ?> media="print">#wpadminbar { display:none; }</style>
looks wrong to me, as $type_attr may contain an attribute + its value (example: type="text/css".

If we want to escape this variable, I think esc_html would be more appropriate here.

@dhrumilk
20 months ago

@audrasjb Please Verify the diff it OK or does need to change.

#4 follow-up: @krupalpanchal
19 months ago

Shouldn't all the changes be in one diff file instead of multiple, like 58102.diff, 58102.2.diff etc. Or is that okay to have multiple diff files?

Because in the 58102.diff, there is a change in the option.php file. In 58102.2.diff, there is a change in admin-bar.php and so on.

#5 in reply to: ↑ 4 @NekoJonez
19 months ago

Replying to krupalpanchal:

Shouldn't all the changes be in one diff file instead of multiple, like 58102.diff, 58102.2.diff etc. Or is that okay to have multiple diff files?

Because in the 58102.diff, there is a change in the option.php file. In 58102.2.diff, there is a change in admin-bar.php and so on.

I think it's best practice to set them all in one file. Otherwise it looks like we have 4 different versions of one patch...

#6 @audrasjb
19 months ago

It really depends on the scope of the patch. Sometimes, it's easier to separate things into multiple small scoped patches.
But yes, when adding multiple patches to one ticket, it's probably better to name them after to scope of the change.
Example: 58102-revisions.diff would probably be a better name for 58102.4.diff.

@viralsampat
19 months ago

I have removed unused variables from above mentioned files and checked phpcs coding standard.

@viralsampat
19 months ago

I have checked above mentioned issue into the theme section and I have resolved it and added patch.

@viralsampat
18 months ago

I have checked another file and added my patch

@viralsampat
18 months ago

I have checked another theme and founds few file. Here, I have added its patch.

@viralsampat
18 months ago

I have checked another theme and founds few file. Here, I have added its patch.

@viralsampat
18 months ago

I have checked another theme and founds few file. Here, I have added its patch.

@viralsampat
18 months ago

I have checked another theme and founds few file. Here, I have added its patch.

@viralsampat
18 months ago

I have checked above mentioned issue and founds few files. Here, I have added its patch.

#7 @sabernhardt
6 weeks ago

This ticket can focus on whether to use sanitize_text_field() (or similar sanitization) for these items:

  1. wp-settings-{$user_id} cookie in wp_user_settings() (option.php, in 58102.diff)
  2. $_SERVER['HTTP_HOST'] and $_SERVER['REQUEST_URI'] in wp_admin_bar_customize_menu() (admin-bar.php, part of 58102.2.diff)
  3. $_SERVER['HTTP_HOST'] and $_SERVER['REQUEST_URI'] in wp_login_form() (general-template.php, part of 58102-general-template.diff)

For the $_SERVER variables, #16858 and #53998 are related.


Other changes proposed in wp-includes patches:

  1. Escaping $type_attr in both wp_admin_bar_header() and _admin_bar_bump_cb() was unnecessary because the variable represented a specific string (or nothing), and [56682] deprecated and replaced those functions.
  2. Both 58102.3.diff and 58102.4.diff proposed escaping the wp_die() message in _show_post_preview() (revision.php), but I did not find any wp_die() messages that escape the translatable string.
  3. [56359] already set strict comparison in revision.php.
  4. 58102-general-template.diff adds esc_html() in get_search_form() and wp_register(), but both functions need to return or output HTML.
  5. [55642] used strict comparison in get_archive_template().
  6. The documentation for get_page_template() should keep the parentheses for locate_block_template().
  7. The 11 global variables for locate_template() are available for use in the separate template file that this function requires.

Let's ignore the bundled theme patches for this ticket.

  • They escape many translations (mostly replacing _e() with esc_html_e()), but #30724 decided against that for bundled themes.
  • They also remove many helpful comments after endif; that identify what ends there.
  • The twentysixteen_resource_hints() function should remain in the theme in case a site uses it (though the // add_filter line could be removed below the resource hints functions in each theme).
  • 58102-twentyfourteen.diff removes html5.js (part of #58836) and adds esc_attr() for header image dimensions (already escaped in [56583]).
  • Patches add global $post; at the top of twentyfourteen/image.php and twentyeleven/header.php.
Note: See TracTickets for help on using tickets.