Page MenuHomePhabricator

Audit uses of empty() and isset() and eliminate unnecessary uses
Open, MediumPublic


During porting, we have likely used empty() and isset() excessively where it is not required.
@Tgr says in a code review:

When something is an object or null, just !$handler is easiest to read. empty() is generally disliked in the PHP world (unless you are checking something that might not exist) because it suppresses errors so it's easy to e.g. make a refactoring mistake where the variable is renamed but in the empty check it remains on the old name, causing misbehavior without an explicit error.

If the variable can take scalar values, an additional reason not to use empty() is that it accepts a somewhat random list of values (null, false, 0, 0.0, '', '0', []) so it's not trivial to guess what the code is checking for.

Let us git grep for empty() and isset() uses and eliminate uses that aren't required.

Event Timeline

ssastry created this task.Jun 24 2019, 6:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 24 2019, 6:21 PM
ssastry triaged this task as Medium priority.Jun 24 2019, 6:21 PM
ssastry moved this task from Backlog to Porting on the Parsoid-PHP board.
Tgr added a comment.Jun 24 2019, 6:42 PM

This would be a lot easier on top of T226428: Convert stdclass-cast objects to classes wherever possible and use associative arrays elswhere as far as possible - isset/empty is usually needed because it is hard to guarantee array/stdclass fields are always set, and classes do not have that problem.

Krinkle added a project: Technical-Debt.
Krinkle moved this task from Unsorted to Migrate / Replace on the Technical-Debt board.
ssastry moved this task from Porting to Post-Port Work on the Parsoid-PHP board.Aug 29 2019, 3:30 PM
ssastry assigned this task to Sbailey.Oct 11 2019, 9:32 PM

Change 543229 had a related patch set uploaded (by Sbailey; owner: Sbailey):
[mediawiki/services/parsoid@master] Replace empty($var) with !$var & !empty($var) with $var

ssastry reopened this task as Open.Oct 16 2019, 1:55 PM
ssastry removed Sbailey as the assignee of this task.
ssastry added a subscriber: Sbailey.

Let us keep this open for a bit longer to see if there are other aspects to address here including creating first class classes for some objects.

Change 543229 abandoned by Sbailey:
Replace empty($var) with !$var & !empty($var) with $var

No significant performance gain versus risks of introducing subtle defects

Aklapper edited projects, added Parsoid; removed Parsoid-PHP.Apr 10 2020, 4:27 PM