WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#10758 closed defect (bug) (fixed)

Code Improvements (hakre's session)

Reported by: hakre Owned by:
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.8.4
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Collection ticket for code improvements for 2.9 containing minor improvements like wrong formattings, unchecked usage of variable names, consts or inexistant array keys or object properties.

Attachments (14)

10758-admin-widgets.patch (1.3 KB) - added by hakre 5 years ago.
10758-wp-trackback.patch (1.4 KB) - added by hakre 5 years ago.
10758-vars.patch (595 bytes) - added by hakre 5 years ago.
10758-admin-post.patch (2.0 KB) - added by hakre 5 years ago.
10758-wp-includes-post.patch (2.1 KB) - added by hakre 5 years ago.
Not a fix of #4365
10758-wp-includes-authortemplateandcapabilties.patch (1.2 KB) - added by hakre 5 years ago.
Testrun with 2.8.4 revealed unchecked conditions.
10758-wp-includes-functions-wp-styles.patch (2.3 KB) - added by hakre 5 years ago.
Some variable hints and docblock comments.
10758-admin-upload.patch (593 bytes) - added by hakre 5 years ago.
10758-wp-includes-comment.patch (1.2 KB) - added by hakre 5 years ago.
Missing checks for set variables.
10758-wp-includes-pluggable.patch (2.6 KB) - added by hakre 5 years ago.
That global variable is not always an object of class WP_User + global and variable hints
10758-wp-comments-post.patch (564 bytes) - added by hakre 5 years ago.
Missing checks for set variables.
10758-wp-admin-admin-footer.patch (573 bytes) - added by hakre 5 years ago.
Missing checks for set variables.
10758.patch (16.5 KB) - added by hakre 5 years ago.
consolidated patch
10758.2.patch (3.5 KB) - added by hakre 5 years ago.
Updated against current head

Download all attachments as: .zip

Change History (32)

comment:1 follow-up: scribu5 years ago

So where is the patch?

hakre5 years ago

comment:2 in reply to: ↑ 1 hakre5 years ago

Replying to scribu:

So where is the patch?

was interupted by a phone call.

hakre5 years ago

hakre5 years ago

hakre5 years ago

comment:3 hakre5 years ago

See #3329 as loose reference for last patch.

hakre5 years ago

Not a fix of #4365

hakre5 years ago

Testrun with 2.8.4 revealed unchecked conditions.

hakre5 years ago

Some variable hints and docblock comments.

comment:4 follow-up: janeforshort5 years ago

Shouldn't these have individual tickets, so they can have descriptive titles and the ticket can be managed effectively by committers (thinking about the commit stream, and that loading up one ticket might slow down getting commits)?

comment:5 Askapache5 years ago

  • Cc webmaster@… added

why hasn't this been committed yet? I almost submitted some of these same patches, as I'm tired of having to modify these files to prevent PHP NOTICE's in my log file.

At least for the wp-trackback.php, no-brainer.

comment:6 in reply to: ↑ 4 hakre5 years ago

  • Summary changed from Code Improvements to Code Improvements (hakre's session)

Replying to janeforshort:

Shouldn't these have individual tickets, so they can have descriptive titles and the ticket can be managed effectively by committers (thinking about the commit stream, and that loading up one ticket might slow down getting commits)?

Well, the title (summary) is infact not very descriptive. This was a session I've done while monitoring an application. I'll change the title accordingly now. I think many (if not all) of the patches are commit ready but a comitter should give feedback which form is preferred (single patch, more tickets etc.).

maybe related: #10861

hakre5 years ago

hakre5 years ago

Missing checks for set variables.

hakre5 years ago

That global variable is not always an object of class WP_User + global and variable hints

comment:7 hakre5 years ago

as reference for the last one:

PHP Notice: Trying to get property of non-object in /is/htdocs/wp1055410_Q291LS7NBM/www/wp-includes/pluggable.php on line 1227

(that entry is reduced, I had that multiple times on a production site)

hakre5 years ago

Missing checks for set variables.

hakre5 years ago

Missing checks for set variables.

comment:8 follow-up: hakre5 years ago

  • Keywords dev-feedback added

Session is closed now. Any feedback on the patches? Should those be combined? Should I or the committer do it?

comment:9 azaozz5 years ago

There are few places that seem wrong. If some variables are not set we should be returning errors not give them default values. For example creating a nonce for user ID of 0 is not going to happen :)

comment:10 hakre5 years ago

Yeah I thought so as well but you must think it to an end: This patch is not about solving programme-design related problem, it's only for checking. The default values chosen are those for unset variables (NULL) which is perfectly well for this patch then.

Maybe for those cases it actually makes no sense (no idea which that are, I have not designed any of those functions) additional tickets reporting those bugs should be opened and then referenced to this one.

comment:11 in reply to: ↑ 8 Askapache5 years ago

Replying to hakre:

Session is closed now. Any feedback on the patches? Should those be combined? Should I or the committer do it?

See wp-trackback.php ticket

hakre5 years ago

consolidated patch

comment:12 hakre5 years ago

I now put all single patches into one patch and updated it for the current trunk's version.

Related: #10131, #10875

comment:13 azaozz5 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [12284].

comment:14 follow-up: azaozz5 years ago

Skipped couple of fixes as they would need logic changes too.

comment:15 hakre5 years ago

Realted: #11504 ; #10875

comment:16 in reply to: ↑ 14 hakre5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to azaozz:

Skipped couple of fixes as they would need logic changes too.

Would have been great if the left-out changes could have been isolated in an additional patch. Pleases remember that an unset variable always returns the value NULL therefore no logic-changes should have appeared in the patch, at least my intention was to not change the actual workflow. Would be great to know at which places you see logic changes needed.


Will try to compile anohter patch that reflects leftout changes against current head.

hakre5 years ago

Updated against current head

comment:17 scribu5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

westi (#11168):

Please don't reopen tickets from previous milestones where a fix was made during that release.

It makes it much harder to keep track of the remaining issue - a new ticket should be created for an issue once the release the original ticket was closed against has shipped.

See #11521.

comment:18 hakre5 years ago

My fault, thanks scribu for taking care.

Note: See TracTickets for help on using tickets.