Skip to content

NEW Add a feature flag to every feature this module applies - #2

Draft
edwilde wants to merge 1 commit into
upgrade/cms-6from
pulls/upgrade/cms-6/add-feature-flags
Draft

edwilde wants to merge 1 commit into
upgrade/cms-6from
pulls/upgrade/cms-6/add-feature-flags

Conversation

@edwilde

@edwilde edwilde commented Sep 10, 2026

Copy link
Copy Markdown

Implements silverstripe#136.

Referenced rather than Fixes, deliberately: this merges into the CMS 6 upgrade branch on the Silverstripe Ltd fork, so the upstream issue should stay open until this reaches silverstripe/cwp-core.

Summary

Every project installing cwp/cwp-core inherits forced SSL, basic authentication, NZISM password rules, session settings, a syslog handler, a queued jobs runner and more. None of it could be turned off without forking the module.

Each of those is now a boolean flag, all 19 listed together in _config/features.yml, all defaulting to true. A project that upgrades and configures nothing behaves as it does today, with one exception under Behaviour changes.

How it works

Two patterns, chosen by whether the feature already has a class.

Features backed by a class carry the flag on that class and guard themselves:

public function process(HTTPRequest $request, callable $delegate)
{
    if (!$this->config()->get('enabled')) {
        return $delegate($request);
    }
    // existing logic
}

Features that are pure YAML get a small class each under CWP\Core\Config, sharing a FeatureToggle trait, with apply() called from _config.php.

The defaults themselves stay in the module's YAML rather than moving into _config.php. Config written from _config.php lands in a higher priority layer than any YAML, so writing the defaults from PHP would silently take away a project's existing override of something like SilverStripe\Control\Session.timeout. So apply() only does work when a flag is off, at which point it undoes that feature's configuration.

silverstripe/config has no primitive for peeling a module's own YAML layer back off: Config::modify()->set() replaces the merged value, and remove() is an array_diff_key, which deletes rather than reverts. Each teardown therefore names the value it expects to find and the value to leave behind, and writes only where the current value still matches what this module set. A project that configured its own value keeps it.

Where that value comes from a backticked environment variable, the value matched against is the unresolved string. Injector resolves backticks when it builds the service, in convertServiceProperty(), and nothing resolves them in the config layer.

With every flag on, no teardown runs and behaviour is byte for byte what it was.

What can be turned off

Six flags sit on classes that already existed: InitialisationMiddleware, CwpBasicAuthMiddleware, RichLinksExtension, CwpHtmlEditorConfig, LoginAttemptNotifications, CwpAtomFeed.

Thirteen new classes under CWP\Core\Config cover CMS styles, session timeout, password field autocomplete, member lockout, locale, password strength, password encryption, SSL enforcement, logging, oEmbed, queued jobs, text extraction and document conversion.

Disabling reads:

CWP\Core\Config\SessionConfig:
  enabled: false

Existing property level toggles such as egress_proxy_default_enabled and xss_protection_enabled are untouched and still honoured, with enabled sitting above them.

Two settings the module applies deliberately carry no flag, because each already matches the framework default and there is nothing to switch off: HTMLEditorField.sanitise_server_side in _config/editor.yml, and Session.cookie_secure on non-dev environments in _config/live-config.yml. Both remain ordinary config a project can set for itself.

Behaviour changes

Password strength. _config/security.yml pointed at SilverStripe\Security\PasswordValidator, which CMS 6 moved to SilverStripe\Security\Validation\, so the NZISM rules were not being applied at all on this branch. It now configures RulesPasswordValidator, which framework 6 does not use by default. Sites go from entropy scoring to a 10 character minimum across 3 of 4 character classes.

That is the intended behaviour, but it stops people setting passwords they can set today. A project that wants the upgrade to change nothing can turn the feature off:

CWP\Core\Config\PasswordStrengthConfig:
  enabled: false

That hands the PasswordValidator service back to the framework's EntropyPasswordValidator, which is what this branch resolves to today, so no password that works now stops working. The NZISM rules can then be turned on as a separate, deliberate change with its own comms.

CustomHtmlEditorFieldToolbar moved to a matching filename. It was declared in src/Extension/CustomHtmlEditorField.php, so it resolved through Silverstripe's class manifest rather than composer's PSR-4 autoloader. It carries no flag: HtmlEditorField_Toolbar, the class _config/extensions.yml applies it to, was removed in CMS 6 along with the updateMediaForm hook, so there is no behaviour for a flag to switch off. Whether it can be ported to the CMS 6 media form belongs to the upgrade rather than to this change.

Removed tests/PasswordEncryptor/PBKDF2Test.php, since the class it covered was removed earlier in the CMS 6 work. Two other tests are brought up to date with CMS 6 namespaces.

Testing

56 tests, 122 assertions, against silverstripe/framework 6.2 on PHP 8.4 and PHP 8.5, with MySQL 8.4. phpcs clean on both. Coverage runs in both directions for each flag: the default still applies while enabled, and the specific fallback holds when disabled.

Deliberately not included

Existing drift, left alone for separate issues:

  • _config/config.yml still configures CWP\Core\Config\CwpInitialisationFilter, a class that no longer exists, and an unqualified GDBackend, gone in CMS 6. Both inert.
  • tests/OEmbedTest.php calls ReflectionProperty::setAccessible(), deprecated in PHP 8.5 and a no-op since 8.1, so the suite emits one deprecation on 8.5.
  • composer.json declares php: ^8.3, but the tree resolves Symfony 8.1, which requires >= 8.4.1, so 8.3 fails Composer's platform check. A packaging decision rather than part of this change.

Every project installing cwp-core inherits forced SSL, basic authentication,
NZISM password rules, session cookie settings, a syslog handler, a queued
jobs runner and more. None of it could be turned off without forking.

Each of those is now a boolean flag in _config/features.yml. Setting one to
false makes _config.php undo that feature's configuration at boot.

silverstripe/config has no primitive for peeling a module's own YAML layer
back off: Config::modify()->set() replaces the merged value, and remove() is
an array_diff_key, which deletes rather than reverts. Each teardown
therefore names the value it wants to leave behind, and writes it only where
the current value still matches what this module set, so a project that
configured its own value keeps it.

Where that value comes from a backticked environment variable, the value to
match is the unresolved string: Injector resolves backticks when it builds
the service, in convertServiceProperty(), and nothing resolves them in the
config layer.
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 this pull request may close these issues.

1 participant