[policy] Coding Standards Update for PHP5 [#576248]

The http://drupal.org/coding-standards page needs some additional info for PHP5 situations, since Drupal 7 now requires it. There are also places in the code that will need to be updated to follow these new conventions, catch in particular.

- Exceptions - see http://drupal.org/node/522746#comment-1992260 as a starting point
- Try / Catch - just like if / else, the } at the end of the try block should be on its own line, then catch(Exception $e) { on a line
- Classes - documentation standards for classes are a little light. Exceptions and other new additions will make classes more common.

There may be others as well.

Comments

Comment #1

Crell CreditAttribution: Crell commented 14 September 2009 at 00:04

Additional recommendations, based on what we're already doing in the DB layer:

- Interfaces should all be named SomethingHereInterface.

- Exceptions should all be named SomethingHereException.

- I'm not sure if we want to flag abstract classes similarly. The proposed framework coding standards (put forth by a couple of guys from some of the big "real" frameworks on a lark and with a lot of resistance from the PHP community, but still worth noting) suggests, I believe, SomethingHereAbstract. In the handlers/plugins skunkworks I've been doing I've used the format SomethingHereBase. I am open to discussion on this point.

- Agreed on catch () {} being on its own line. That's consistent with the rest of core and with what DBTNG is doing.

I also just noticed the coding standard recommendation for using underscores to prefix protected/private variables in classes. I don't know who thought that was a good idea, but it's not. :-) We're not doing that anywhere AFAIK. It's a hold-over from PHP 4 when there were no explicit private/protected flags, and it just makes code harder to read. It should be removed.

Comment #2

kwinters CreditAttribution: kwinters commented 14 September 2009 at 12:51

If we're going to give Abstract classes a suffix, it should probably be Abstract rather than Base (which doesn't really have any common meaning). Also, something can be a base class without being abstract.

The private class member vars being prefixed with an underscore is probably an extension of "private" functions in modules, which are used all over. It does seem unfortunate in a class though.

Comment #3

jrchamp CreditAttribution: jrchamp commented 14 September 2009 at 13:03

Subscribing. Nice suggestions so far.

Comment #4

Crell CreditAttribution: Crell commented 19 October 2009 at 00:33

Also relevant: #608124: Link function parameter types and return types to class/interface pages

Comment #5

Crell CreditAttribution: Crell commented 19 October 2009 at 02:18

I've added two docs pages on OO and Exceptions. Both are still marked incomplete while we review, but they cover what was discussed here (sort of) plus other stuff based on DBTNG.

http://drupal.org/node/608152
http://drupal.org/node/608166

Comment #6

kwinters CreditAttribution: kwinters commented 19 October 2009 at 13:17

Re: OO

The Interfaces and Visibility sections (others?) make sense for situations like DBTNG but maybe not for Drupal modules or even some places in core because many of those objects aren't really meant to be extended directly. Also, the typical use pattern for objects in the past was really just as data storage (db results) so I imagine that will continue. I'm fine with a more structured OO approach, but I'd wager there will be resistance.

Instantiation really needs an example. Not clear from the docs: the factory should be a public class member function?

The main coding standards page also had some updates, which look good. However, the naming standards could still use the rest of the info from the OO page (interfaces, etc.) or even better a link for more info. I just don't want anyone to see what's there and not notice / look for the link at the bottom.

Comment #7

Crell CreditAttribution: Crell commented 19 October 2009 at 16:40

I added a direct link from the main coding standards page to the child page.

For the factory, eh, we don't have a standard yet. I almost always use either a separate function or a separate function that loads a factory class (a factory factory approach, yes I went there!), but some other core systems are using a static method.

We also don't have a good standard yet for how to handle static methods. :-) Personally I try to avoid them always, as they do not inherit nicely in PHP 5.2 due to early static binding.

IMO, "an object that isn't really meant to be extended directly" is often a sign of a design flaw. If you're using a class and not allowing for alternate implementations, you're missing half the reason to use a class.

stdClass objects are mostly unaffected by this since they're glorified arrays that pass in a funny way. I'm not sure how we want to cover that.

Comment #8

kwinters CreditAttribution: kwinters commented 19 October 2009 at 17:14

Is this what you're referring to re: binding? http://socket7.net/article/php-5-static-and-inheritance and http://php.net/manual/en/language.oop5.late-static-bindings.php

Since the behavior is different in PHP 5.2 and 5.3, I'd agree that we should generally avoid static member functions at least until 5.3+ is required. I'm not sure how well it applies to factory functions, though, since they very naturally go with the class object itself so it might be the lesser of evils. It's kind of a shame that the API freeze has passed, since it would be good to make the end solution consistent across core :(

Hooks really cloud the whole inheritance issue in Drupal, but in general I'd agree that any classes defined should be designed for extensibility. I think the fight will happen over bloat vs extensibility, but I generally side with extensible.

However, we might be able to still use public member vars extensibly without getters / setters using PHP Magic Methods for getters / setters. This would also allow more contexts to treat stdClass and custom objects the same (foreach, etc.).

Comment #9

Crell CreditAttribution: Crell commented 6 November 2009 at 18:03

Bloat: Features I don't need.

Feature: Bloat that I need.

:-)

Hooks don't cloud inheritance, since they operate orthogonally to inheritance. That's what's so cool about them. They're a fast, implicit form of observer or visitor patterns.

I actually am very skeptical about magic get/set. They have their uses and I have used them before, but they can also easily lock you into a corner, especially as they don't 100% emulate bare properties so you often still need to know what you're accessing. For foreach(), it's not get/set that is useful but the ArrayAccess interface, which I definitely agree is useful.

Comment #10

kwinters CreditAttribution: kwinters commented 6 November 2009 at 18:50

I mean that there isn't a standard for using a hook inside of a class versus overriding that class, or guidelines describing when either is appropriate. Or for that matter, when you should even use a class at all rather than do all extensibility through hooks. That's probably more of a documentation issue than anything.

Do we have enough reason to recommend getters / setters outright (and private member vars)? Or I guess we could just list pros and cons, and give some example reasons for choosing A or B.

Comment #11

Crell CreditAttribution: Crell commented 7 November 2009 at 00:31

Honestly, in practice I've found I generally don't need "bean-style" get/set methods. If you have a perfect 1:1 mapping, you should consider whether you really want a class in the first place. (You still may, but possibly not.)

For a more completed take on when to use OO vs. not, that's being somewhat discussed over here: #326881: Object Oriented Design documentation needs updating

More intricate stuff like how to comingle hooks and classes, I don't know yet. More experimentation is required.

Comment #12

sdboyer CreditAttribution: sdboyer commented 11 November 2009 at 20:47

For foreach(), it's not get/set that is useful but the ArrayAccess interface, which I definitely agree is useful.

Err...I think you're thinking of something that extends Traversable (e.g., something implementing Iterator). Unless there's something REALLY squirrelly and hidden away in there, fulfilling the ArrayAccess interface doesn't make an object foreach()-able.

Comment #13

Crell CreditAttribution: Crell commented 11 November 2009 at 20:54

Hm. You are correct. I'm thinking of the various Iterator interfaces, which in turn extend Traversable. ArrayAccess is something else (that is also cool but in a different way).

Comment #14

donquixote CreditAttribution: donquixote commented 2 June 2011 at 14:02

I'm not sure if this is the correct issue for this, but:
We need to decide how class names should be prefixed. See
http://drupal.org/node/608152#comment-3683598

("quote" modified a bit)

It is important both for class names and method names to know which module they belong to.
Assuming a module called your_module, the CamelCase convention would probably result in something like
YourModuleSomeClass

This totally sucks, because
a) the module name is not "YourModule", but "your_module".
b) it is hard to tell from the class name, if the module name is "Your"/"your" or "YourModule"/"your_module", or "YourModuleSome"/your_module_some. Yes, this same problem already exists for regular functions, but why not do something better for classes then...

I usually use your_module_SomeClass, to indicate which part is the prefix and which part is the class name.
Sometimes _your_module_SomeClass for "private" classes that are not meant to be instantiated outside of the module with respect to future api changes.

I do not insist on the "private class" thing.
The idea was that a private class should not be directly instantiated or subclassed from outside the module. However, there could still be a public factory function that returns instances of the private class.
For private classes it can be considered safe to change signature between module versions, or even change the class name. Or, if other modules get in touch with instances, then the signature / interface should be stable, but the constructor behavior and the class name can safely change between versions.

I can open a new issue for this, if you like.