[RFC|Accepted] Coding Guideline Modifications

These requests for comments/change have lead to an implemented feature that has been successfully merged into the 3.1/Ascraeus branch. Everything listed in this forum will be available in phpBB 3.1.
Post Reply
User avatar
naderman
Product Manager
Product Manager
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Karlsruhe, Germany
Contact:

[RFC|Accepted] Coding Guideline Modifications

Post by naderman » Fri Jun 25, 2010 12:00 am

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

User avatar
igorw
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: [RFC] Coding Guideline Modifications

Post by igorw » Fri Jun 25, 2010 1:43 am

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.

User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC] Coding Guideline Modifications

Post by bantu » Fri Jun 25, 2010 9:50 am

  • "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().

User avatar
FeyFre
Registered User
Posts: 29
Joined: Wed Mar 17, 2010 9:49 pm

Re: [RFC] Coding Guideline Modifications

Post by FeyFre » Fri Jun 25, 2010 9:55 am

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.

User avatar
bantu
3.0 Release Manager
3.0 Release Manager
Posts: 557
Joined: Thu Sep 07, 2006 11:22 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC] Coding Guideline Modifications

Post by bantu » Fri Jun 25, 2010 9:56 am

FeyFre wrote: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

User avatar
igorw
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: [RFC] Coding Guideline Modifications

Post by igorw » Fri Jun 25, 2010 10:35 am

bantu wrote:
FeyFre wrote: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
Registered User
Posts: 122
Joined: Tue Nov 11, 2008 5:44 pm

Re: [RFC] Coding Guideline Modifications

Post by Nelsaidi » Fri Jun 25, 2010 3:49 pm

FeyFre wrote: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.

User avatar
naderman
Product Manager
Product Manager
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC] Coding Guideline Modifications

Post by naderman » Sat Jul 03, 2010 1:44 pm

eviL3 wrote: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.
eviL3 wrote:
+ <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?
evil3 wrote:
sanitize
Maybe "sanitise"? Not that I'd really care.
Used all over the document, don't really see the need to change that.
evil3 wrote:
+ <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.
evil3 wrote:
+ <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.
evil3 wrote:
+ <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.

User avatar
igorw
Registered User
Posts: 500
Joined: Thu Jan 04, 2007 11:47 pm

Re: [RFC] Coding Guideline Modifications

Post by igorw » Sat Jul 03, 2010 2:22 pm

naderman wrote: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.

User avatar
naderman
Product Manager
Product Manager
Posts: 1727
Joined: Sun Jan 11, 2004 2:11 am
Location: Karlsruhe, Germany
Contact:

Re: [RFC] Coding Guideline Modifications

Post by naderman » Sun Jul 04, 2010 11:02 am

Oh, right ;-)

Post Reply