Page MenuHomePhabricator

Don't use PHP empty()
Closed, ResolvedPublic

Description

I see this extension makes use of empty() in quite a few places.

$ ack empty -Q --php -A3
DataCollector.php
122:		if ( empty( $licenseData['LicenseShortName'] ) ) {
123-			$problems[] = 'no-license';
124-		}
125:		if ( empty( $informationData['ImageDescription'] ) ) {
126-			$problems[] = 'no-description';
127-		}
128:		if ( empty( $informationData['Artist'] ) && empty( $informationData['Attribution'] ) ) {
129-			$problems[] = 'no-author';
130-		}
131:		if ( empty( $informationData['Credit'] ) && empty( $informationData['Attribution'] ) ) {
132-			$problems[] = 'no-source';
133-		}
134-
--
368:		if ( empty( $licenses ) ) {
369-			return array();
370-		}
371-
--
390:		if ( empty( $deletions ) ) {
391-			return array();
392-		}
393-		return $deletions[0];

TemplateParser.php
127:		if ( !$html ) { // DOMDocument does not like empty strings
128-			return array();
129-		}
130-

The empty() function is one of PHP's worst and has a track record keeping a steady flow of small and big bugs in the MediaWiki landscape. Its behaviour also changed throughout PHP past releases.

empty( $var ) essentially does !isset( $var ) || !$var. It's unlikely the author wanted to account for the property potentially not existing, and also had to account for all the things that cast to boolean false in PHP (no control over the type of value being null, array, int, float, boolean, or string).

Use of empty() makes it hard to distinguish intentional tolerance from common errors and provides no advantage over other approaches. It makes for code that is hard to understand, maintain, support, debug and iterate upon.

Please use isset() and/or other more strict assertions instead.

Event Timeline

Krinkle created this task.Dec 13 2014, 2:58 AM
Krinkle assigned this task to Tgr.
Krinkle raised the priority of this task from to Normal.
Krinkle updated the task description. (Show Details)
Krinkle changed Security from none to None.
Krinkle added subscribers: Krinkle, Tgr.

Change 179860 had a related patch set uploaded (by Gergő Tisza):
Replace unneeded empty() calls

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

Patch-For-Review

Tgr added a comment.Dec 14 2014, 11:08 PM

In most of those snippets, the code does test for a combination of the array field being unset and being the empty string. I think

empty( $informationData['Artist'] ) && empty( $informationData['Attribution'] )

is much easier to read than

!isset( $informationData['Artist'] ) || !isset( $informationData['Attribution'] ) || $informationData['Artist'] === '' || $informationData['Attribution'] === ''
aaron added a subscriber: aaron.Dec 14 2014, 11:11 PM

There are cases where empty() is fine. One example is checking boolean flags that may or may not be set.

In this case, we want to check if strings are not empty. empty() doesn't work well since the string could be "0". Using empty() is bug here isn't it?

Tgr added a comment.Dec 15 2014, 1:02 AM

I guess so, although '0' is not a meaningful value for any of these fields, but it could still cause confusion.
Updated the patch.

Gilles moved this task from Untriaged to Ready for testing on the Multimedia board.
Gilles added a subscriber: Gilles.

Change 179860 merged by jenkins-bot:
Replace unneeded empty() calls

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

Gilles closed this task as Resolved.Dec 18 2014, 8:49 AM
Gilles moved this task from Doing to Done on the Multimedia board.