Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update PHP coding standards to avoid using self #486

Closed
5 tasks done
GuySartorelli opened this issue Mar 26, 2024 · 3 comments
Closed
5 tasks done

Update PHP coding standards to avoid using self #486

GuySartorelli opened this issue Mar 26, 2024 · 3 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Mar 26, 2024

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

  • The PHP coding standards are updated to explicitly disallow using self (e.g. self::get())
  • 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
  • static is fine, because it explicitly uses late static binding anyway
  • References to self in our codebase are updated to use the actual class that is intended to received the call.
  • Update code-writing script and re-run to use self::class::myMethod() in traits

Code writing script

Kitchen sink CI

PRs

@maxime-rainville
Copy link
Contributor

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
Copy link
Member Author

Is there an actual issue that got cause by this?

Yes. I'll link you the slack thread.

@GuySartorelli GuySartorelli added this to the Silverstripe CMS 5.3 milestone May 3, 2024
@emteknetnz emteknetnz self-assigned this Jun 5, 2024
This was referenced Jun 5, 2024
@emteknetnz emteknetnz removed their assignment Jun 14, 2024
@GuySartorelli GuySartorelli self-assigned this Jun 16, 2024
@GuySartorelli GuySartorelli changed the title Update PHP coding standards to avoid calling static methods with self Update PHP coding standards to avoid using self Jun 16, 2024
@GuySartorelli
Copy link
Member Author

PRs merged.
I've raised silverstripe/silverstripe-standards#8 to add a rule in CI to catch this happening in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants