Update PHP coding standards to avoid using `self`

GuySartorelli commented 8 months ago

We had a bug which it turned out was caused by an unexpected interaction between self and late static binding (see this PR). This was raised in php-src where the response was that it was working as intended.

Going forward we should avoid calling static methods from self to avoid this class of bug.

Notes

In refinement we decided it's easiest to just avoid using self altogether, rather than only avoiding it for calling methods

Acceptance Criteria

[x] The PHP coding standards are updated to explicitly disallow using self (e.g. self::get())
[x] The docs mention the rationale, i.e. we're avoiding a specific class of bug that is introduced when using late static binding downstream from self
[x] static is fine, because it explicitly uses late static binding anyway
[x] References to self in our codebase are updated to use the actual class that is intended to received the call.
[x] Update code-writing script and re-run to use self::class::myMethod() in traits

Code writing script

https://github.com/emteknetnz/self-remover

Kitchen sink CI

https://github.com/emteknetnz/recipe-kitchen-sink/actions/runs/9514611296

PRs

maxime-rainville commented 7 months ago

Is there an actual issue that got cause by this? e.g. Did we have a scenario where we called self::get() in Link wanting to query all Links and ended up querying only FileLink.

GuySartorelli commented 7 months ago

Is there an actual issue that got cause by this?

Yes. I'll link you the slack thread.

GuySartorelli commented 5 months ago

PRs merged. I've raised https://github.com/silverstripe/silverstripe-standards/issues/8 to add a rule in CI to catch this happening in the future.

silverstripe / developer-docs