Page MenuHomePhabricator

Don't use PHP empty()
Closed, ResolvedPublic


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

$ ack empty -Q --php -A3
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-		}
368:		if ( empty( $licenses ) ) {
369-			return array();
370-		}
390:		if ( empty( $deletions ) ) {
391-			return array();
392-		}
393-		return $deletions[0];

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

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 assigned this task to Tgr.
Krinkle raised the priority of this task from to Medium.
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


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'] === ''

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?

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

Gilles moved this task from Doing to Done on the Multimedia board.