[RFC|Accepted] Coding Guideline Modifications

[naderman]

So we've already been operating with slightly modified coding guidelines but we should formalise those. I have created a patch of the coding-guidelines.html file which you can find here. The changes are:

• Class names need to be prefixed with phpbb_
• eval should not be used in any form
• there should be newlines at the end of file
• the closing php tag should be ommited
• array elements should always have a trailing comma
• the phpBB VCS is now git
• removed the coding guidelines changelog

[igorw]

Looks pretty good. But what about the class naming proposal, which also includes things like "there should only be one class per file"?

+ <p>If an array is defined with each element on its own line, you still have to modify the previous line to add a comma when appending a new element. PHP allows for trailing (useless) commas in array definitions. These should always be used so each element including the comma can be appended with a single line</p>

You might want to add a trailing full stop there for consistency.

sanitize

Maybe "sanitise"? Not that I'd really care.

+ <li><code>master-phpbb2</code><br />A branch containing all stable phpBB2 release points</li>

I'd omit that or at least make clear that it is dead. "stable" implies otherwise.

+ <li><strong>tags</strong><br />Released versions. Merged into the master if it is a stable release.

Are tags merged into a branch? The wording seems a bit off to me.

+ <li><code>release-3.Y.X-RCY</code><br />Release candidate Y of the stable 3.Y.X release.</li>

I'd rather use Z than using Y twice. Might be worth mentioning PLs too.

[bantu]

• "public, private, protected" should be used for class properties instead of "var"
• Methods that are called statically should have the "static" keyword
• Use "static public function foo()" instead of "public static function foo()"
• Prefer class constants over define().

[FeyFre]

I know, it was discussed, but problem was not solved

the closing php tag should be ommited

What about MODs dependent on it? <?php and ?> was only stable elements in sources, which will never be modified by developers and other mods? Now only <?php which is unusable.

[bantu]

I know, it was discussed, but problem was not solved

the closing php tag should be ommited

What about MODs dependent on it? <?php and ?> was only stable elements in sources, which will never be modified by developers and other mods? Now only <?php which is unusable.

Discussion was here: viewtopic.php?f=3&t=32589
Related ticket: http://tracker.phpbb.com/browse/PHPBB3-9556

[igorw]

I know, it was discussed, but problem was not solved

the closing php tag should be ommited

What about MODs dependent on it? <?php and ?> was only stable elements in sources, which will never be modified by developers and other mods? Now only <?php which is unusable.

Discussion was here: viewtopic.php?f=3&t=32589
Related ticket: http://tracker.phpbb.com/browse/PHPBB3-9556

I have posted a suggestion in that topic, please comment.

[Nelsaidi]

I know, it was discussed, but problem was not solved

the closing php tag should be ommited

What about MODs dependent on it? <?php and ?> was only stable elements in sources, which will never be modified by developers and other mods? Now only <?php which is unusable.

The ommited ?> would ONLY be at end of file, I'm sure detection can be easily done too.

I like these updates, cant see any faults apart from evil's sugestion of one class per fil which is as important.

[naderman]

Looks pretty good. But what about the class naming proposal, which also includes things like "there should only be one class per file"?

Added that. Including the singular/plural thing.

+ <p>If an array is defined with each element on its own line, you still have to modify the previous line to add a comma when appending a new element. PHP allows for trailing (useless) commas in array definitions. These should always be used so each element including the comma can be appended with a single line</p>

You might want to add a trailing full stop there for consistency.

Trailing full stops are a syntax error, so I don't quite see the point?

sanitize

Maybe "sanitise"? Not that I'd really care.

Used all over the document, don't really see the need to change that.

+ <li><code>master-phpbb2</code><br />A branch containing all stable phpBB2 release points</li>

I'd omit that or at least make clear that it is dead. "stable" implies otherwise.

Removed it.

+ <li><strong>tags</strong><br />Released versions. Merged into the master if it is a stable release.

Are tags merged into a branch? The wording seems a bit off to me.

Yes, the tags are merged into a branch. Rephrased it anyway.

+ <li><code>release-3.Y.X-RCY</code><br />Release candidate Y of the stable 3.Y.X release.</li>

I'd rather use Z than using Y twice. Might be worth mentioning PLs too.

That was a mistake, fixed.

[igorw]

Trailing full stops are a syntax error, so I don't quite see the point?

What I meant was adding a full stop after "These should always be used so each element including the comma can be appended with a single line".

Looks good.

[naderman]

Oh, right