Page MenuHomePhabricator

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

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 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.

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.

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

https://gerrit.wikimedia.org/r/543229

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

Reason:
No significant performance gain versus risks of introducing subtle defects

https://gerrit.wikimedia.org/r/543229

Change 756172 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/services/parsoid@master] Initialize 'inTemplate' pipeline option always and fix usages

https://gerrit.wikimedia.org/r/756172

Change 756172 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Initialize 'inTemplate' pipeline option always and fix usages

https://gerrit.wikimedia.org/r/756172

Change 758585 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.15.0-a18

https://gerrit.wikimedia.org/r/758585

Change 758585 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.15.0-a18

https://gerrit.wikimedia.org/r/758585