WordPress.org

Make WordPress Core

Opened 20 months ago

Last modified 7 months ago

#43755 reviewing defect (bug)

CS: Fix violations for wp-includes/canonical.php

Reported by: jipmoors Owned by: netweb
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Canonical Keywords: has-patch needs-refresh
Focuses: coding-standards Cc:
PR Number:

Description

Started working on this at WordCamp London contributor day.

Attachments (2)

43755-fix-coding-standards.patch (13.9 KB) - added by jipmoors 20 months ago.
43755-fix-coding-standards-2.patch (11.2 KB) - added by jipmoors 7 months ago.
Removed added type-checks and placed declarations before if-statements where possible

Download all attachments as: .zip

Change History (10)

#1 @netweb
20 months ago

  • Component changed from General to Canonical
  • Keywords needs-patch added

#2 @netweb
20 months ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to netweb
  • Status changed from new to reviewing

#3 @jipmoors
20 months ago

Have made some additional changes to make sure wp_parse_url is used on all occasions.

Also to be able to ignore the prepare statement sniff, have made sure all parts of the query have been prepared.
I can imagine this could be a separate patch or at least be a separate commit, but it was in line of fixing the code sniff.

Have ran all unit tests and did not encounter any problems.

#4 @jipmoors
20 months ago

  • Keywords has-patch added; needs-patch removed

#5 @netweb
14 months ago

  • Milestone changed from 5.0 to 5.1

#6 @pento
11 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.1 to 5.2

This is going to need some careful review. A couple of notes from my brief read through:

When moving an assignment out of a conditional block, it should be moved to before the conditional, unless there are significant performance reasons to move it inside.

eg:

if ( is_feed() && ( $id = get_query_var( 'p' ) ) ) {
        // ...
}

would become:

$id = get_query_var( 'p' );
if ( $id && is_feed() ) {
        // ...
}

There's no need to add an empty() call where there wasn't one before.

Similarly, there's no need to add extra type checks. If the value returned by get_permalink() is truth-y, you can assume it's a string.

#7 @pento
8 months ago

  • Milestone changed from 5.2 to Future Release

@jipmoors
7 months ago

Removed added type-checks and placed declarations before if-statements where possible

#8 @jipmoors
7 months ago

Please note: I was not (yet) able to test through all the changes myself yet.

Note: See TracTickets for help on using tickets.