Manual talk:Coding conventions/PHP

Jump to: navigation, search

Assignment expressions[edit]

This is neither an error nor is it surprising, and in 5.3x allows direct access to referenced element:

$this->assertInstanceOf( 'ExpectedClass', $obj = new $testClass );

- Amgine (talk) 05:10, 25 August 2012 (UTC)

Same to me. It s neither surprising nor is space cheap (as my displays are always having less lines than desirable for a good code structure overview) and it is much much less readable. You do not forbid $x++ either, do you? --Purodha Blissenbach (talk) 09:16, 2 October 2015 (UTC)

"comment should be put on its own line." ?[edit]

This rule can be found in the Spaces section of these conventions. Apparently it was moved here in r547162. I can't really find the related text in the old rules in the Coding conventions diff between before and after the move so this has been added during the move, please correct me if I am wrong. So does this rule make any sense? Comments are used in the same line with code all over the core code. --Danwe (talk) 12:58, 14 September 2012 (UTC)

Since nobody seems to know about this, I have just removed the rule again as described above. --Danwe (talk) 10:01, 27 September 2012 (UTC)

Spacing within brackets[edit]

Judging by the examples, I guess we don't use spacing within brackets; so use, e.g., $options['q'] rather than $options[ 'q' ], eh? Leucosticte (talk) 17:39, 10 November 2012 (UTC)

Correct. I've noticed several extensions incorrectly leave space around $wgHooks keys, e.g. $wgHooks[ 'BeforePageDisplay' ][] = ... -- S Page (WMF) (talk) 06:29, 6 July 2013 (UTC)

Ternary contradiction[edit]

It says we don't allow it because it only works in PHP 5.3, before noting we now require PHP 5.3. So the only reason to forbid it would be for reasons of readability.

I think it's a useful occasional shortcut, so we should allow it. But if we want to forbid it, it should just say that's a convention we're setting. Superm401 - Talk 06:52, 11 January 2013 (UTC)

@param format[edit]

There are two ways given to use @param:

  1. @param type $varname: description
  2. @param datatype1|datatype2 $paramname description

The first one specifically says, "Multiple types can be listed by separating with a pipe character." so, the only real difference is the colon (ignoring the space for now).

The second is correct. To show examples, isUtf8 (generated doc, code) does it that way, and it is correct in the HTML. The type is italicized on the left (if you inspect it, you'll see the HTML class is "paramtype". There is no extra comma for each parameter in the generated docs.

For an example with colons, you can look at a method called outputFileHeaders (generated doc, code). It has two parameters, one strictly matching the first, and the other with an extra space before the colon.

But both the ways with colon are wrong. You see both colons and in the HTML, and there's a spurious comma after one. There's a lot more inconsistency in other methods. Bottom line, I propose we use:

@param datatype1 $paramName description

It's the same as #2 above, except I made the parameter name match our conventions, and removed the second type. To be complete, we can also show:

@param datatype1|datatype2 $paramName description

I'm not proposing a big fix up commit, just that we do this for new code and when we're already touching existing code. Superm401 - Talk 07:30, 11 January 2013 (UTC)

+1 But what about @param $paramName datatype1|datatype2 description I think it is also used and seems to work fine. --Danwe (talk) 22:19, 11 January 2013 (UTC)
It is used, but it actually does not work (maybe it once did, e.g. with PHPDoc) right. Example, Article::_construct (generated doc, code). Unlike isUtf8], the types are not in the right place, nor do they use the paramtype HTML class. They're just crammed into the description. Superm401 - Talk 14:20, 12 January 2013 (UTC)

Control structures[edit]

This page currently says this:

Opinions differ as to whether control structures if, while, for, foreach etc. should be followed by a space; the following two styles are acceptable:

// Spacey
if ( isFoo() ) {
	$a = 'foo';

// Not so spacey
if( isFoo() ) {
	$a = 'foo';

Is it time to pick one, at least for new code?

Some statistics: in mediawiki/core, it seems about 84.7% uses the spacy style. Broken down by keyword:

Keyword Spacey Not-so-spacey % spacey
elseif 1394 250 84.7932
for 231 67 77.5168
foreach 1987 413 82.7917
if 14797 2476 85.6655
switch 138 116 54.3307
while 297 74 80.0539
TOTAL 18844 3396 84.7302

It looks to me like we should standardize on "spacey". Anomie (talk) 18:58, 22 January 2013 (UTC)

+1. --Amir E. Aharoni (talk) 19:06, 22 January 2013 (UTC)
Support. --Krenair (talkcontribs) 19:26, 22 January 2013 (UTC)
Support I think we already use spaces where most people don't (inside parens). If we now decided not to use them where most people do (after control structures) I would really call our coding conventions perverted. So +1 for spacey here. --JGonera (WMF) (talk) 19:37, 22 January 2013 (UTC)
Oppose A space before the first opening parenthesis should match all other function calls, instead of having a separate one. Think of the new developers - we break away from most coding practices of other projects, shouldn't we try to minimize the differences rather than introduce more of them? There is no need to have a unique one for control structures - they are already highlighted by the IDE and followed by the indented block. --Yurik (talk) 21:58, 22 January 2013 (UTC)
Can you provide examples of popular open source projects which don't use spaces after control structures? I just checked the first that came to my mind (Linux kernel) and it does have spaces. On the PHP front, the most popular web frameworks (Symfony, Kohana, lithium) also use spaces. --JGonera (WMF) (talk) 22:40, 22 January 2013 (UTC)
Funny - I just checked linux kernel and gnu specs too, and they have a space after if (), BUT they do not have spaces inside the (), which balances the visual of the parens. A quick google code search shows over 4 million hits for various projects, but this number is significantly lower than the one with the space. Again, it's not a fair comparison because they don't have spaces inside. What I meant initially was that I don't know any major projects that puts spaces both inside and outside, so we should try to get rid of at least one to match. --Yurik (talk) 01:56, 23 January 2013 (UTC)
Support I'm fine with the status quo, which is also consistent with our JS coding style. Let's not change the standard away from what 84% of the code already uses. --Catrope (talk) 22:08, 22 January 2013 (UTC)
Stylize.php also converts forcefully to spacey format. --Nikerabbit (talk) 07:43, 23 January 2013 (UTC)
Spacey is a bit closer to PSR-2, which the PHP world seems to be moving towards. --Tgr (talk) 11:38, 26 January 2013 (UTC)
Support Platonides (talk) 16:52, 27 January 2013 (UTC)
Support I really don't care as long as we pick one. Parent5446 (talk) 05:27, 27 February 2013 (UTC)

When I wrote the document, I left the question open out of respect for Brion, who wrote code using the not-so-spacey style. I was blind to the difference until I wrote stylize.php, which demonstrated differences between my style (which stylize.php represented) and code written by Brion. -- Tim Starling (talk) 06:53, 13 May 2013 (UTC)

Recommending some minimum documentation practices[edit]

I would like to propose adding the following paragraph to the beginning of the "Comments and Documentation" section:

It is essential that your code be well commented so that other developers and bug fixers can easily navigate the logic of your code. This is especially important for an open source project like MediaWiki where technical complexity can be a deterrent to volunteer collaboration. To make your code easier to navigate, it is recommended that you add comments providing brief descriptions of new classes, methods, and member variables (unless the functionality is obvious). It is also recommended that new methods include parameter and return definitions when applicable.

Kaldari (talk) 23:57, 5 July 2013 (UTC)

(ec) this seems like something that pretty much went without saying. The only part i would remove is deterent to volunteer contribution. Decent docs benefit everyone. I wouldnt want it to come across like we are only documenting things to appease the pesky volunteers Bawolff (talk) 00:25, 6 July 2013 (UTC)
Honestly, this is supposed to be an enforced document, so I'd prefer if we worded things a bit more strongly. For example, something like:

New methods must have their return type and parameters' types and values documented using Doxygen conventions. New methods should also have a description of what the function does or, if an abstract function, what the function is expected to do. Additionally, all code should be commented as necessary to allow easy understanding of the code's intentions.

My motivation for this is because I'm getting sick of all these abstract classes without any clear documentation as to how to inherit them. Parent5446 (talk) 00:20, 6 July 2013 (UTC)
Code should be clear and easy to understand. Comments can help with this. They are however best used as a last resort, when your efforts to make the code be obvious on its own, failed. To quote Fowler "When you feel the need to write a comment, first try to refactor the code so that any comment becomes superfluous." I recommend keeping comments to a minimum, as they can easily end up hurting readability. "Gets the user name" is a useless comment for method "getUserName", and ends up just cluttering the code, teaching the reader to ignore the comments. Comments also tend to go out of date, get moved about, and often end up being disinformation. So lets please not have some absolutist policy stating that every method should have a comment. That is going to hurt more then help. --Jeroen De Dauw (talk) 01:25, 6 July 2013 (UTC)
The main types of comments are prologue and inline. The first should always be used on public methods, non-trivial protected methods or ones meant to be overriden. Classes themselves should have prologue comments too. That second should always be used for clever/tricky things or things that are done for very non-obvious reasons, remote the code in question (e.g. some subtle aspect of another system). They can also be used for non-obtrusive and quick local variable docs (e.g. " list of (a => b)") and various other, more-optional, reasons. All this IMO of course, so I tend to agree with having some minimum standard. And codifying it is nice since it is obviously not common sense for MediaWiki. Aaron (talk) 03:21, 6 July 2013 (UTC)

Agreed. Personally I think we should build tools to support this. For instance if there was a script that auto-compiled documentations for all active extensions this would encourage adding documentation by highlighting where it is missing. I'd suggest breaking out the conversation around documentation of methods from 'commenting'. Commenting in my opinion should be used as little as possible. Documentation by standard. Jdlrobson (talk) 20:27, 8 July 2013 (UTC)

I've added a new intro paragraph to the section based on the feedback here. It emphasizes documentation over inline comments and uses slightly stronger language than my original wording. Kaldari (talk) 00:36, 10 July 2013 (UTC)

Interview, narrative, lessons-learned approaches[edit]

I read in the research in Making Software that the main thing people often wish they could find in documentation, but can't, is the rationale behind a particular decision. Why was it architected that way? What other approaches did the author consider? Was this a workaround, a bugfix, an experiment? They want the backstory, the narrative, to understand how this fits in with the larger norms of the developer community, and important events. And if the author doesn't happen to explain that in a commit message, a Gerrit code review comment, or a Bugzilla thread, then the reader can't get at it without finding the author and asking, months later.

I don't know where the best place is to put stuff like that. A commit summary might be best, as long as git blame works well. Some approaches:

  • Interview. Ask another developer to pretend to interview you about what you chose and why, and write down the answers.
  • Narrative. Some recommendations say "every commit message should be a user story readable by a nontechnical executive" but in most cases that goes too far. Instead it might work to have a developer walk through the narrative of their development process in their commit summary: "I started by thinking foo, then I ran across problem bar, so I modified this to account for baz."
  • Lessons learned. In a commit summary, include a lesson you learned about this bit of the codebase.

This is a bit weird but might be helpful. Sharihareswara (WMF) (talk) 06:24, 27 July 2013 (UTC)

Triple-equal operator[edit]

I was under the assumption that === is preferred to == unless there's a reason to use ==. Now that I checked I don't see that written here. Should it be written? --Amir E. Aharoni (talk) 13:35, 2 December 2013 (UTC)

Yes. --siebrand (talk) 13:50, 2 December 2013 (UTC)


Is the use of braces mandated within the MW coding standard? For instance, it seems like this:

 foreach ( $res as $row ) {
     showRow( $row );

is more common than this:

 foreach ( $res as $row )
     showRow( $row );

and this:

 if ( $i == 1 ) {
 } else {

is preferred to this:

 if ( $i == 1 ) {
 else {

Can someone comment? Should it be part of this document? @User:Sharihareswara_(WMF) Rs561 (talk) 19:10, 21 May 2014 (UTC)

@Rs561: It is mandated, yes (personally I dislike the style we've adopted, but such is life). Not sure how best to document. Jdforrester (WMF) (talk) 19:57, 21 May 2014 (UTC)
I see someone went and added this information to this page. I replaced it with a reference to Manual:Coding conventions#Indenting and alignment, since there's no need to have two copies of the information that would get out of sync. Anomie (talk) 13:18, 23 May 2014 (UTC)

Static Binding[edit]

In PHP 5.3, a new feature called "Late Static Binding" (LSB) was added to help work around this perceived lack of functionality in static functions. However, the usefulness of LSB is debatable among MediaWiki developers and should be avoided for the time being.

I believe we should use static in new code written to avoid issues with binding while leaving self on old code to avoid possible bugs. static has the binding behaviour on php that you would expect if you come to it from other languages NRuiz (WMF) (talk) 16:51, 22 May 2014 (UTC)
I see several uses of "new static" or "static::" in core. I wouldn't necessarily go recommending using static over self when people don't know why they're using static instead of self though. Anomie (talk) 13:13, 23 May 2014 (UTC)

Namespaces vs. prefixes for classes[edit]

I've seen a few instances of namespaced class names like "WikiGrok\Api\ApiWikiGrokResponse" or "OAuth\Frontend\SpecialPages\ SpecialOAuthRegistrationFrontend" that combine both prefixing/suffixing and namespacing. This seems totally redundant. Would it be worth adding something to the Classes section to discourage this redundancy? Kaldari (talk) 18:47, 23 October 2014 (UTC)

I'm not seeing "OAuth\Frontend\SpecialPages\SpecialOAuthRegistrationFrontend" anywhere in mediawiki/core or mediawiki/extensions. Nor the ApiWikiGrokResponse class, but that may be because WikiGrok doesn't seem to be in either of those repos yet either. Anomie (talk) 13:30, 24 October 2014 (UTC)

Recent edits[edit]

Krinkle recently made two major changes to the coding conventions:

  • Forbidding use of empty(), based on the fact that he doesn't trust people to use it correctly. When used when !isset( $foo ) || !$foo would otherwise be used, I don't see any reason to avoid it since that's its explicit purpose.
  • Forbidding casting of arrays to booleans, apparently for the same reason. I doubt the claimed advantage to using count() is worth the tiny cost of the additional function call.

Personally, I'd rather not mess around with the extra verbosity. But let's discuss. Anomie (talk) 22:32, 15 December 2014 (UTC)

Do interfaces start with an I?[edit]

Going forward, should interfaces start with an I (e.g. IDatabase), or not (e.g TitleFormatter)? Mattflaschen-WMF (talk) 15:21, 8 September 2016 (UTC)

In general, I'd say no. Sometimes it makes sense if the interface is being split from an existing base class, but often enough it's as unnecessary as prefixing member fields with "m". Anomie (talk) 13:20, 9 September 2016 (UTC)

Can we enforce no-spaces-after-cast?[edit]

The guidelines here say that we "do not use a space within or after the cast operator", but this is not currently enforced by the codesniffer rules. There seems to be a bit of a long history of discussing this, and the Generic.Formatting.NoSpaceAfterCast and Generic.Formatting.SpaceAfterCast rules have been added and removed at various times (see here and here for some of the discussion).

I can't see the advantage to having this not tested one way or the other by phpcs, so would like to submit a patch that re-adds Generic.Formatting.NoSpaceAfterCast to the MediaWiki CS ruleset. I thought I'd bring it up here first though, because obviously there's more to this than meets the eye! :-)

Thanks! Sam Wilson 03:23, 28 October 2016 (UTC)

I don't see any reason not to. Glancing through the links, it seems only Krinkle has stated objection to the current text of this page and has unilaterally blocked enforcing the style in phpcs. If there has been other discussion on this issue where others have supported Krinkle's position, links would be appreciated.
For the record, "no spaces after a cast" was added in February 2013, removed (by Krinkle) in June 2013, and readded in August 2013. It seems to have stood since then. Anomie (talk) 13:48, 28 October 2016 (UTC)
Okay cool! I've created task T149544 and submitted Gerrit change 318880. Sam Wilson 04:36, 31 October 2016 (UTC)

Spaces inside declare()[edit]

I think declare statements should be mentioned in the CC. Should they be formatted like normal function calls?

Example 1:

declare( strict_types = 1 );

I prefer this option as it would be more in alignment with the "space-heavy" style of the rest of the convention.

Alternatively, we could adopt a more terse style, to make the declarations kind of stand out:


What do you think?

--Gabriel Birke (WMDE) (talk) 14:56, 12 April 2017 (UTC)

At the moment there's probably little reason for us to use declare since we haven't dropped support for PHP5 and PHP5-compatible versions of HHVM. But if we do use it, as we do here, it should not be a strange exception to the spacey style we use everywhere. The only question would be whether it should be treated as a control structure or not, i.e. declare( ... ); and declare( ... ) { ... } versus declare ( ... ); and declare ( ... ) { ... }. Anomie (talk) 13:24, 13 April 2017 (UTC)
Apparently, declare goes back to PHP 4, though I've never seen it before. It is a "flow control construct[]", per docs, so it should be spacy, exactly like if. encoding was added in 5.3 (though I don't see why we would want to use it), and strict_types is added in PHP 7 (and thus won't be available for a while). Mattflaschen-WMF (talk) 05:06, 18 April 2017 (UTC)

Declaring multiple properties with same public/private/protected statement[edit]

PHPCS is currently OK with lines like:

private $mBlacklist = null, $mWhitelist = null;

What about the following (taken from TitleBlacklist):

		$mRaw,           ///< Raw line
		$mRegex,         ///< Regular expression to match
		$mParams,        ///< Parameters for this entry
		$mFormatVersion, ///< Entry format version
		$mSource;        ///< Source of this entry

Should the latter be allowed? This was originally reported as phab:T166381. Legoktm (talk) 19:17, 6 June 2017 (UTC)

I'd say not. It makes more sense to format these in the normal way:
	/** @var string Raw line. */
	private $mRaw;

	/** @var string Regular expression to match. */
	private $mRegex;

	/** @var string Parameters for this entry. */
	private $mParams;

	/** @var string Entry format version. */
	private $mFormatVersion;

	/** @var string Source of this entry. */
	private $mSource;

Sam Wilson 23:47, 6 June 2017 (UTC)

@Legoktm, Samwilson: It should be formatted as given at Manual:Coding_conventions/PHP#.40var:_documenting_class_members, or another way with equivalent Doxygen formatted (HTML) output. I am certain the first two examples do not have proper output (since they have no types). I have tested similar examples to Sam's, and believe that also won't render correctly at (correct me if I'm wrong, though). Mattflaschen-WMF (talk) 22:50, 7 June 2017 (UTC)
@Mattflaschen-WMF: hmm yes good point, I always forget how different Doxygen is to the rest of the PHP world! I now remember having read about this problem before. Sam Wilson 22:56, 7 June 2017 (UTC)
Thanks, Sam. That bug links to a StackOverflow answer, which might be helpful to avoid the redundancy (repeating the variable name). We already have an input filter; that could be added if it's reliable enough to handle our code base. However, we should make sure the format the filter accepts matches what IDEs expect (for type-hinting, etc.) Mattflaschen-WMF (talk) 23:48, 7 June 2017 (UTC)
BTW, I can't reproduce the bug (needing to repeat $fieldName) as of today. Not sure the exact situation or what changed. Mattflaschen-WMF (talk) 20:36, 28 June 2017 (UTC)