Problem/Motivation

So, what is the best way to approve/document such evolutions to our coding standards, while still accommodating existing code to not adhere to them?

Proposed resolution

?

Remaining tasks

Discuss

Comments

effulgentsia created an issue. See original summary.

klausi’s picture

From https://www.drupal.org/coding-standards

Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version. Existing code in older versions may be updated, but doesn't necessarily have to be. Especially for larger code-bases (like Drupal core), updating the code of a previous version for the current standards may be too huge of a task. However, code in current versions should follow the current standards.

Keep it simple: only one version of coding standards with current best practices. Old code may violate it, which is ok.

If there are special cases just mention it in the standards:
* Short array syntax: "Note that code written for Drupal 7 and before should not use short array syntax because it should be compatible with PHP 5.3 which does not have the short array syntax."
* Parameter type hinting: "Note that for backwards compatibility reasons existing code without type hints is allowed."

David_Rothstein’s picture

I agree with @klausi - there are good reasons the coding standards are version-agnostic (simplicity, less confusion when backporting patches, etc) and exceptions to that for "technical" reasons are probably rare enough that it makes sense to just document those inline with the specific coding standard.

If I were to suggest any changes it might be to this:

Existing code in older versions may be updated, but doesn't necessarily have to be. Especially for larger code-bases (like Drupal core), updating the code of a previous version for the current standards may be too huge of a task.

Which is a little wishy-washy. We should probably at least strongly encourage the updating of old code (at least for Drupal core) after a new coding standard is adopted, even if it isn't a strict requirement. Because if contributors adopt a new coding standard when writing new code but the old code is never updated, then the codebase just winds up being inconsistent.

pfrenssen’s picture

I personally don't think we should have different versions of the coding standards. I fully agree with @klausi that it should not be expected that people have to update old code to comply to new standards:

Keep it simple: only one version of coding standards with current best practices. Old code may violate it, which is ok.

In addition to this, I don't think we ever need to hold back on adding a new rule to Coder because "some old code might not be compatible". There are ways to handle use cases where a particular project needs to ignore certain newly added coding standards rules:

  • If there are isolated cases where a coding standards change might affect backwards compatibility (like #1158720: [policy, no patch] Add parameter type hinting to function declaration coding standards) then you can use the @codingStandardsIgnoreStart and @codingStandardsIgnoreStart tags to exclude the affected section from coding standards checks, preferably with a short explanation of why the standard is not being followed.
  • If a standards change affects large parts of the code base (e.g. #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8) then you can follow the example of core and provide a custom phpcs.xml.dist file that excludes the incompatible sniff for the entire code base.
  • Custom code written for client projects can specify the version of Coder which was used at the time the project was initially written, and lock this in your make file (for Drupal 7) or composer.lock file (for Drupal 7/8). This will effectively "lock in" the coding standard which was in use at the time the bulk of the code was written, and will reduce the maintenance cost of keeping up with coding standards changes to zero.

These potential B/C breaking changes are quite rare, so the burden of maintaining these exclusions is low.

I do think that potentially B/C breaking changes should come with a warning. Like @klausi suggested:

"Note that for backwards compatibility reasons existing code without type hints is allowed."

quietone’s picture

Category: Task » Support request
Priority: Major » Normal
Status: Active » Fixed

There has been no discussion here for 7 years and the three comments support the existing practice that 'Drupal coding standards are version-independent and "always-current"'. #4 also provides options for working with 'old' code.

The Issue Summary asks for clarification, therefore I am changing to a support request. I am also closing as fixed because of the 3 answers.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.