DOC Update coding conventions

[emteknetnz]

Issue #323

Right now it requires some discussion with @silverstripe/core-team to see everyone's happy with the conventions that I've added in < This has been done

This PR will only focus on PHP. I've spun off a separate card for JS + SCSS

[michalkleiner]

👍 Generally agree with all the proposed additions. It'd be great to have linter/phpcs/PhpStan rules for as many of these as possible.

[GuySartorelli]

There's some stuff here that seems like a duplication of the stuff below your additions - what's the intention there? Will you be removing all the old standards? Will you do a de-dupe separately? Or are the duplications not intended?

I'll hold off reviewing the existing standards until you've answered this.

I also assume that the other language coding standards have yet to be done.

[ScopeyNZ]

Mostly feel strongly about trying to ratify member visibility, as some may remember from silverstripe/silverstripe-framework#8996.

[maxime-rainville]

There's a lot of good stuff in there!!!

I would make the following assumptions explicit once and avoid repeating them over and over:

• Best practices are approaches that will work well in most situations.
• It's OK to occasionally ignore them. (Maybe we want to have two classes ... the ones that are nice to have but not so important ... and the ones that should only be ignored as a last results)
• Best practices have not always been systematically applied, so you will encounter instances where they are not followed.

In term of language, I would be inclined to try to normalise it and avoid complicated dissertation. Maybe remove in-depth explanation of why we think something is a good.

I would focus on using the words "avoid" or "prefer" in most places. Maybe avoid using expression like "where possible" or "if practical".

Some extra questions we might want to address:

• Do we prefer Promises or Async/Await in JS?
• Prefer using the ORM instead of raw SQL.
• Prefer DataObjectSchema::tableName() to get the table name for DataObject rather then hardcode the value.
• Prefer generic/standard SQL syntax that work across all DataBase.

[ScopeyNZ]

Do we prefer Promises or Async/Await in JS?

I'm against mentioning a preference here. They have different use-cases. Both are situational, and may even be used together, eg:

const thing = await fetch('url').then(response => response.json());

Prefer DataObjectSchema::tableName() to get the table name for DataObject rather then hardcode the value.

This should be a rule (at least it was when I was last peer-reviewing 😅 )

[emteknetnz]

Have made recommended edits. Have also updated the content that was previously on the page and either made it much more concise or removed. Ready for review again

[GuySartorelli]

@maxime-rainville in #397 (review) you said

Maybe avoid using expression like "where possible" or "if practical".

There's still some of that in here so you may want to look and see if you're okay with the way that language has been used in the new revision.

[GuySartorelli]

Still not done:

• DOC Update coding conventions #397 (comment)
• DOC Update coding conventions #397 (comment)

[GuySartorelli]

@maxime-rainville here are the specific things you need to double check:

• DOC Update coding conventions #397 (comment)
• DOC Update coding conventions #397 (review)

[maxime-rainville]

Maybe avoid using expression like "where possible" or "if practical".

There's still some of that in here so you may want to look and see if you're okay with the way that language has been used in the new revision.

I had a glance and didn't see anything obviously standing out. By general point was that "where possible" or "if practical" is kind of implied when drafting best practices.

[GuySartorelli]

LGTM. No outstanding requested changes.