count() and sizeof() are synonymous in functionality, the latter being an alias of the former. In Drupal's code base count() is used 631 times and sizeof() only 10 times. For consistency I propose we kill of those ten and then update the coding standards to discourage using sizeof().

Including a patch to fix the ten sizeof() uses.

CommentFileSizeAuthor
#11 change-sizeof-to-count-1509968-11.patch2.21 KBdcam
#6 change-sizeof-to-count-1509968-6.patch4.12 KBdcam
#4 change-sizeof-to-count-1509968-4.patch8.24 KBdcam
drupal-consistently-use-count-instead-of-sizeof-alias.patch4.6 KBtobiassjosten

Comments

mikeryan’s picture

mikeryan
he/him
Primary language English
Location Pittsfield, MA, USA
commented
Status: Active » Needs review
kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented

drupal-consistently-use-count-instead-of-sizeof-alias.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal-consistently-use-count-instead-of-sizeof-alias.patch, failed testing.

dcam’s picture

dcam commented
Status: Needs work » Needs review
StatusFileSize
new change-sizeof-to-count-1509968-4.patch8.24 KB

I rerolled the patch in the OP and added a few more changes where sizeof() was used in Symfony and Doctrine tests.

droplet’s picture

droplet commented
Status: Needs review » Needs work

- code style problem
- don't hack 3rd part (eg. vendor)

dcam’s picture

dcam commented
Status: Needs work » Needs review
StatusFileSize
new change-sizeof-to-count-1509968-6.patch4.12 KB

Yeah, I noticed the code style problems too. I didn't know if patches should be kept on task and separate issues should be opened for formatting or if those problems should be fixed where they're found. The whole file, core/lib/Drupal/Component/Archiver/ArchiveTar.php, is riddled with style errors.

As for the 3rd party code changes, I knew better. I don't know why I second-guessed myself.

droplet’s picture

droplet commented

ArchiveTar is a modified 3rd party script. #1414508: update system.tar.inc to latest Archive_Tar (Pear)

dcam’s picture

dcam commented

In that case should it also be excluded?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Correct, we should not change ArchiveTar.php as part of this issue.

ryan.ryan’s picture

ryan.ryan commented

Patch applied cleanly, replacements were made in 8 locations. Looks good.

dcam’s picture

dcam commented
Status: Needs work » Needs review
StatusFileSize
new change-sizeof-to-count-1509968-11.patch2.21 KB

Removed ArchiveTar.php changes.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Confirmed these are the only three lines in modules/ using sizeof, and we should not change any others.

tobiassjosten’s picture

tobiassjosten commented

Good that you amended the patch (RTBC indeed). But I don't see the urgency that you wouldn't allow me to finish what I started?

dries’s picture

dries commented
Priority: Normal » Minor
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.