PHP coding standard

jdillard

The source of that page:

http://web.archive.org/web/20130310191034/http://devwiki.pfsense.org/DeveloperRules/showcode

phil.davis

Thanks, I remember reading that in the past but had not been able to find it.

jimp

I'm working on recovering and updating that, I'll have it up on the doc wiki shortly, from there we can work on bringing it more up-to-date and adding more to it. It's quite dated and some things have changed significantly. For example it mentions nowrap, which was changed to be a CSS class and not a tag.

jimp

Here it is:

https://doc.pfsense.org/index.php/Developer_Style_Guide

Though most of the content is up for debate. Input welcome.

phil.davis

When creating if statements, even if there is only one statement inside, the use of braces is preferred.

+++
Yes, yes, yes - in past projects I have been involved in we also made this a rule. Too often there was an "if" followed by a single (indented) statement with no {} curlies. Some later sucker who was not wide awake at the time would add another (indented) statement, intending it to be inside the "if" clause. It all looks beautifully indented but the interpreter (or compiler) does not care about that pretty formatting and happily executes the 2nd statement outside the "if".

Same should apply to other code-block structures like "for" "foreach"…

BBcan177

@jimp:

Here it is:

https://doc.pfsense.org/index.php/Developer_Style_Guide

Though most of the content is up for debate. Input welcome.

So for the next 30 days we will call it the "JIMP Marathon" to clean up code and there will be no questions asked!! but after that …. ;D ;D

jimp

I'm afraid that one is probably not going to be done by me any time soon. :-)

We keep threatening to rewrite everything in something that isn't PHP. That would be the natural time to clean things up.

phil.davis

I don't think we want to format/cleanup code modules that have come from other places, because doing that makes it more difficult in future to compare the version in pfSense with a current version of the module/class… and find the fixes that would be good to take into pfSense.
Examples in /etc/inc :
CHAP.inc
IPv6.inc

and there is simplepie.inc and I'm sure a bunch of others that we do not really want to massage into the pfSense coding style guide.

That could be mentioned in the Style Guide wiki page.

phil.davis

@jimp:

I'm afraid that one is probably not going to be done by me any time soon. :-)

We keep threatening to rewrite everything in something that isn't PHP. That would be the natural time to clean things up.

I have finished coding style guide changes fro everything under /etc - found a couple of little bugs while inventing editor macros and regex to find and update routine format stuff.
It does seem worthwhile to take a sweep through, it will at least mean that people new to the project will be able to contribute by copy-paste of the code format they find and not have to wonder which is the official way it should be done.
Next week I might get enthused again and start on /usr…

Cargo

Usefull information. Had thoughts like that. Thnx.

doktornotor

I did some packages cleanup in past couple of days… Some notes:

1/ Where the hell is some documentation? Like, even a darned list of the supported tags? Nothing?
2/ A template package would help as well. Though, not the broken test_package in the repo...

phil.davis

I left this alone for a while after completing an initial pass. Unfortunately it overlapped with the time when the bootstrap conversion was going on "somewhat independently" in another GitHub account/repo and that repo did not entirely keep up with integration with all the code style changes that happened in master. After the integration of the bootstrap conversion stuff into master, various bits of formatting reverted to their previous state (although a lot of it did also survive).

Since then, not everybody has been committing code that matches the developer style guide.

I was looking at some widget stuff today, and as well as a couple of PRs for little fixes, I did a sweep through and made minor format and white space changes to match the style guide.

It seems that the rate of change in master is beginning to slow - perhaps we are getting close to a stable code base again, with only localized bugs to be found - no more things that need sweeping re-engineering.

That being so,

Is there much care factor about the style guide?
If I make PRs to get the code back closer to the style guide, is that OK?
Or will it mess up someone who has another repo somewhere with a lot of pending changes that would get dumb conflicts?

jimp

@phil.davis:

Is there much care factor about the style guide?

I can't speak for others, but I definitely care, I've been fixing things as I find them but I haven't been intentionally seeking them out.

@phil.davis:

If I make PRs to get the code back closer to the style guide, is that OK?
Or will it mess up someone who has another repo somewhere with a lot of pending changes that would get dumb conflicts?

I don't know that anyone has any big changes like that pending currently, since things are settling down it would likely be a good time to start on those sorts of things.

phil.davis

What should the standard be for tabbing in parameters on function calls that span multiple lines?
i.e. when stringing the function name and all the parameters in one line makes the line "too long".

e.g.

Try to "line up" the additional parameters under the first parameter.
a) By putting enough TAB and space characters assuming that each TAB is being displayed at 4-space intervals in some editor:

TAB do_stuff($parameter1,
TAB TAB TAB space "Second parameter is some literal text",
TAB TAB TAB space $parameter3);

b) By putting enough TAB and space characters assuming that each TAB is being displayed at 8-space intervals in some editor:

TAB do_stuff($parameter1,
TAB TAB space "Second parameter is some literal text",
TAB TAB space $parameter3);

c) By putting TAB characters to the function name then space characters to fill out under the function name:
This avoids any assumption about the editor TAB display mode.

TAB do_stuff($parameter1,
TAB 9space "Second parameter is some literal text",
TAB 9space $parameter3);

Forget about lining up under the parameter on the first line, always indent subsequent lines by 1 TAB:

TAB do_stuff($parameter1,
TAB TAB "Second parameter is some literal text",
TAB TAB $parameter3);

Put the function name on a line by itself and all the parameters on later lines, always indented by 1 TAB:

TAB do_stuff(
TAB TAB $parameter1,
TAB TAB "Second parameter is some literal text",
TAB TAB $parameter3);

and if one of the parameters has some really long literal stuff, like long text, then extra lines that continue the same parameter are indented an extra TAB:

TAB do_stuff(
TAB TAB $parameter1,
TAB TAB gettext("Second parameter is some literal text that goes on and on and has line breaks.") . '
' .
TAB TAB TAB gettext("There is a lot to explain to the user.") . " " .
TAB TAB TAB gettext("So we need to concatenate a lot of text together into a single parameter."),
TAB TAB $parameter3);

TAB do_stuff(
TAB TAB $parameter1,
TAB TAB gettext("Second parameter is some literal text that goes on and on and has line breaks.") . '
' .
TAB TAB TAB gettext(
TAB TAB TAB TAB "There is a lot to explain to the user. " .
TAB TAB TAB TAB TAB "So we need to concatenate a lot of text together into a single parameter. " .
TAB TAB TAB TAB TAB "And sometimes the parameters passed are themselves a function that spans multiple lines."),
TAB TAB $parameter3);

The code currently uses various ways to do it, and it would be nice if there was a standard so that there does not need to a banter about it with each pull request!

Personally I like (3)+(4).