Adhere to best practices for getCMSFields() across supported modules in CMS 6

[GuySartorelli]

As part of silverstripe/silverstripe-cms#2767, new best practices are established regarding the scaffolding of form fields for the CMS in CMS 6.

1. all DataObject subclasses should always call parent::getCMSFields() in their getCMSFields() method.
   • If they don't want scaffolded form fields, they can set mainTabOnly to true in their scaffold_cms_fields_settings config
   • If they don't want to use tabs, they can set tabbed to false in their scaffold_cms_fields_settings config
2. beforeUpdateCMSFields() should always be used, with the exception of changes to form fields that are explicitly not to be available in extensions
   • This was a pre-existing best practice that we just haven't enforced across our supported modules yet
3. Where form fields are scaffolded, but not wanted, ignoreRelations and ignoreFields options in scaffold_cms_fields_settings config should be used rather than removeByName()

I've taken a look and found the following in our supported modules where these best practices are not being followed:

Known to be avoiding parent::getCMSFields():

• SilverStripe\Security\Group
• SilverStripe\Assets\File
• SilverStripe\Blog\Model\BlogObject
• SilverStripe\SiteConfig\SiteConfig
• SilverStripe\Subsites\Model\SubsiteDomain
• SilverStripe\UserForms\Model\EditableFormField
• SilverStripe\UserForms\Model\Recipient\EmailRecipient
• SilverStripe\Versioned\ChangeSet
• Symbiote\AdvancedWorkflow\DataObjects\WorkflowAction
• Symbiote\AdvancedWorkflow\DataObjects\WorkflowDefinition
• Symbiote\AdvancedWorkflow\DataObjects\WorkflowInstance
• Symbiote\AdvancedWorkflow\DataObjects\WorkflowTransition
• TractorCow\Fluent\Model\Domain
• TractorCow\Fluent\Model\FallbackLocale
• TractorCow\Fluent\Model\Locale

Known to not be using beforeUpdateCMSFields():

• SilverStripe\Security\PermissionRole
• SilverStripe\LDAP\Model\LDAPGroupMapping
• SilverStripe\SpamProtection\EditableSpamProtectionField
• Symbiote\AdvancedWorkflow\Actions\AssignUsersToWorkflowAction
• Symbiote\AdvancedWorkflow\Actions\NotifyUsersWorkflowAction
• Symbiote\AdvancedWorkflow\Actions\PublishItemWorkflowAction
• Symbiote\AdvancedWorkflow\Actions\SetPropertyWorkflowAction
• Symbiote\AdvancedWorkflow\Actions\UnpublishItemWorkflowAction
• Symbiote\QueuedJobs\DataObjects\QueuedJobDescriptor (also using removeByName() instead of scaffold_cms_fields_settings config)

Known to be using removeByName() when this could be configured using scaffold_cms_fields_settings config:

• SilverStripe\Security\Member
• SilverStripe\LinkField\Models\EmailLink
• SilverStripe\LinkField\Models\Link
• SilverStripe\LinkField\Models\PhoneLink
• SilverStripe\LinkField\Models\SiteTreeLink
• SilverStripe\Subsites\Model\Subsite
• SilverStripe\Taxonomy\TaxonomyTerm
• SilverStripe\UserForms\Model\EditableFormField\EditableCountryDropdownField
• SilverStripe\UserForms\Model\EditableFormField\EditableDropdown
• SilverStripe\UserForms\Model\EditableFormField\EditableFieldGroup
• SilverStripe\UserForms\Model\EditableFormField\EditableFieldGroupEnd
• SilverStripe\UserForms\Model\EditableFormField\EditableFileField
• SilverStripe\UserForms\Model\EditableFormField\EditableFormHeading
• SilverStripe\UserForms\Model\EditableFormField\EditableFormStep
• SilverStripe\UserForms\Model\EditableFormField\EditableLiteralField
• SilverStripe\UserForms\Model\EditableFormField\EditableMemberListField
• SilverStripe\UserForms\Model\EditableFormField\EditableRadioField
• SilverStripe\UserForms\Model\Submission\SubmittedForm
• DNADesign\Elemental\Models\BaseElement

Acceptance criteria

• Across all supported modules, the best practices described above are followed
• If there are exceptions where those best practices don't apply, comments in the code clearly point out why they're not following those best practices
• A new PHPStan rule is created in silverstripe/standards to catch scenarios where parent::getCMSFields() is not called within a getCMSFields() implementation in a DataObject subclass
  • This must explicitly not result in an error for DataObject::getCMSFields() itself
• A new PHPStan rule is created in silverstripe/standards to catch scenarios where $this->beforeUpdateCMSFields() is not called within a getCMSFields() implementation in a DataObject subclass
  • This must explicitly not result in an error for DataObject::getCMSFields() itself
  • If this results in too many instances of PHP comments ignoring this rule, discuss with the CMS Squad whether it's worth it or not
• A new PHPStan rule is created in silverstripe/standards to catch scenarios where removeByName() is being used instead of scaffold_cms_fields_settings config
  • This is run on both DataObject and Extension subclasses
  • It is based on configuration in the class hierarchy only (i.e. yaml is ignored, and extension config isn't considered part of data object config)