Maniphest T195123

Add or enable rule for spaces inside index brackets
Closed, DuplicatePublic
Actions

Assigned To

None

Authored By

	Krinkle
	May 19 2018, 10:41 PM

Description

I noticed in https://gerrit.wikimedia.org/r/#/c/434108/3/includes/libs/rdbms/lbfactory/LBFactory.php that the file contains the following code without violations detected by Codesniffer:

$x = [
	'IPAddress' => isset( $_SERVER[ 'REMOTE_ADDR' ] ) ? $_SERVER[ 'REMOTE_ADDR' ] : '',
	'UserAgent' => isset( $_SERVER['HTTP_USER_AGENT'] ) ? $_SERVER['HTTP_USER_AGENT'] : '',
];

Note in particular that one line uses spaces within the ['key'] access, and the other does not. Ideally, we would standardise on one of these, at least for the simple cases where the key is a literal string or number.

Event Timeline

Krinkle created this task.May 19 2018, 10:41 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 19 2018, 10:41 PM

I think originally no preference for either style was explicitly stated until https://www.mediawiki.org/w/index.php?diff=2227991&oldid=2202539&title=Manual:Coding_conventions/PHP

One relevant discussion I found was https://www.mediawiki.org/wiki/Manual_talk:Coding_conventions/PHP#Spacing_within_brackets which wasn't really a discussion...

My personal preference is $array['key']. I feel this illustrates the intent better than having the key separated off with extra spaces. It's not like the array key is an optional argument, or there can be two in the same set of brackets. So what do we win by having 6 non-alphanumeric characters around a key ([ '' ]) compared to 4 ([''])? The single quotes already give the key room (when using a monospaced font, which I assume is the default).

On the other hand, inlined constructs like $array[$foo->bar()][$whatever->thisIsGettingOutOdHand()] clearly need more structure. But are the issues with code like this solved just by adding spaces? I honestly don't think so.

Because of this I would ask to either disallow these spaces, or keep the status quo of them being optional.

• Vvjjkkii renamed this task from Add or enable rule for spaces inside index brackets to 8mcaaaaaaa.Jul 1 2018, 1:09 AM

• Vvjjkkii triaged this task as High priority.

• Vvjjkkii added projects: CheckUser, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), Tamil-Sites, Gamepress, Hashtags, Jade, KartoEditor, Language-2018-Apr-June, New-Editor-Experiences, Mail, TCB-Team (now WMDE-TechWish).

• Vvjjkkii updated the task description. (Show Details)

• Vvjjkkii removed a subscriber: Aklapper.

CommunityTechBot renamed this task from 8mcaaaaaaa to Add or enable rule for spaces inside index brackets.Jul 1 2018, 5:34 PM

CommunityTechBot raised the priority of this task from High to Needs Triage.

CommunityTechBot updated the task description. (Show Details)

CommunityTechBot removed projects: TCB-Team (now WMDE-TechWish), Mail, New-Editor-Experiences, Language-2018-Apr-June, KartoEditor, Jade, Hashtags, Gamepress, Tamil-Sites, Connected-Open-Heritage-Batch-uploads (RAÄ-KMB_1_2017-02), CheckUser.

CommunityTechBot added a subscriber: Aklapper.

xSavitar subscribed.Apr 6 2019, 8:52 PM

Umherirrender closed this task as a duplicate of T249872: Spaces inside brackets used for array indexing.Jan 1 2021, 3:16 PM

Add or enable rule for spaces inside index bracketsClosed, DuplicatePublicActions

Description

Event Timeline

Add or enable rule for spaces inside index brackets
Closed, DuplicatePublic
Actions