Page MenuHomePhabricator

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

Description

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 Normal 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.Thu, Aug 29, 3:30 PM