Manual talk:Coding conventions/PHP

From MediaWiki.org
Jump to: navigation, search

Assignment expressions[edit | edit source]

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)

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

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 | edit source]

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 | edit source]

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 | edit source]

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 | edit source]

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 Support. --Krenair (talkcontribs) 19:26, 22 January 2013 (UTC)
Support 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 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 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 Support Platonides (talk) 16:52, 27 January 2013 (UTC)
Support 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 | edit source]

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 | edit source]

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 | edit source]

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)

Braces[edit | edit source]

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 ) {
     foo();
 } else {
     bar();
 }

is preferred to this:

 if ( $i == 1 ) {
     foo();
 }
 else {
     bar();
 }

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 | edit source]

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 | edit source]

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 | edit source]

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)