Make WordPress Core

Opened 6 years ago

Closed 13 months ago

#43755 closed defect (bug) (duplicate)

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

Reported by: jipmoors's profile jipmoors Owned by: netweb's profile netweb
Milestone: Priority: normal
Severity: normal Version:
Component: Canonical Keywords: has-patch
Focuses: coding-standards Cc:

Description

Started working on this at WordCamp London contributor day.

Attachments (2)

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

Download all attachments as: .zip

Change History (11)

#1 @netweb
6 years ago

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

#2 @netweb
6 years ago

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

#3 @jipmoors
6 years 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
6 years ago

  • Keywords has-patch added; needs-patch removed

#5 @netweb
5 years ago

  • Milestone changed from 5.0 to 5.1

#6 @pento
5 years 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
5 years ago

  • Milestone changed from 5.2 to Future Release

@jipmoors
5 years ago

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

#8 @jipmoors
5 years ago

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

#9 @SergeyBiryukov
13 months ago

  • Keywords needs-refresh removed
  • Milestone Future Release deleted
  • Resolution set to duplicate
  • Status changed from reviewing to closed

This appears to be resolved in [47759] / #49542. I missed that there was an existing ticket, sorry for that!

Note: See TracTickets for help on using tickets.