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

Method ordering #88

Open
dpi opened this issue Feb 26, 2025 · 2 comments · May be fixed by #90
Open

Method ordering #88

dpi opened this issue Feb 26, 2025 · 2 comments · May be fixed by #90

Comments

@dpi
Copy link
Member

dpi commented Feb 26, 2025

SlevomatCodingStandard.Classes.ClassStructure has been enhanced. We can now add more detailed method ordering rules now that some issues/PR solved:

Proposal 1

Summary of proposed rules:

  • top:
    • test setUp()
    • controller create()
    • plugin createInstance()
    • entity baseFieldDefinitions()
    • invoke method
    • form getFormId(), form getEditableConfigNames()
  • middle
    • form
      • form (content entity form)
      • build
      • validate
      • submit
  • bottom:
    • form actions(), form save()
    • test tearDown()
    • service getSubscribedEvents()
    • __toString()
  • test providers will not be bumper against their method. Theres no rule for this. I think it should be okay that these static methods are moved below the public method block.
  • method order doesnt have context of the class, eg whether its an entity or form or service. So we're just hoping things fall into line.
  • ordering
    • mixed bag of methods, setter/getters, custom methods, etc
    • static methods
    • protected methods
    • private methods

Proposal 2 / Alternate

As visible in a demo PR (not linked), theres quite a large LOC change for proposal 1.

Taking from proposal, but changing some things:

Defer or reconsider static/protected/private ordering.

And actions() / save().

Most others would keep their position simply due to discipline.

dpi added a commit that referenced this issue Feb 26, 2025
@dpi dpi linked a pull request Feb 26, 2025 that will close this issue
dpi added a commit that referenced this issue Feb 26, 2025
@acbramley
Copy link
Contributor

Generally -1 to this, I like the idea but it's too prescriptive

@dpi
Copy link
Member Author

dpi commented Feb 27, 2025

@acbramley mainly I think private/public/static are the most disruptive, the other rules are essentially what we are disciplined enough to follow anyway. I've modified IS with an alternate

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

Successfully merging a pull request may close this issue.

2 participants