Code Improvements (hakre's session)

[hakre]

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.

[scribu]

So where is the patch?

[hakre]

Replying to scribu:

So where is the patch?

was interupted by a phone call.

[hakre]

See #3329 as loose reference for last patch.

[hakre]

Not a fix of #4365

[hakre]

Testrun with 2.8.4 revealed unchecked conditions.

[hakre]

Some variable hints and docblock comments.

[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)?

[Askapache]

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.

[hakre]

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

[hakre]

Missing checks for set variables.

[hakre]

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

[hakre]

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)

[hakre]

Missing checks for set variables.

[hakre]

Missing checks for set variables.

[hakre]

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

[azaozz]

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

[hakre]

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.

[Askapache]

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

[hakre]

consolidated patch

[hakre]

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

Related: #10131, #10875

[azaozz]

Fixed in [12284].

[azaozz]

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

[hakre]

Realted: #11504 ; #10875

[hakre]

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.

[hakre]

Updated against current head

[scribu]

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.

[hakre]

My fault, thanks scribu for taking care.