Skip to content

feat(validator): add settings constraint validation pilot#7077

Closed
somethingwithproof wants to merge 12 commits into
Cacti:developfrom
somethingwithproof:feat/develop-symfony-validator-settings
Closed

feat(validator): add settings constraint validation pilot#7077
somethingwithproof wants to merge 12 commits into
Cacti:developfrom
somethingwithproof:feat/develop-symfony-validator-settings

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

@somethingwithproof somethingwithproof commented Apr 28, 2026

Current Status

Consolidated into #7189. Keep this PR only as historical review context for the original Symfony Validator settings pilot; the merge direction should be the DI-friendly component baseline in #7189.

Original Scope

  • Piloted Symfony Validator behind Cacti-owned settings validation code.
  • Proved the adapter approach before broader Symfony component adoption.

Recommendation

Do not merge this independently unless it is deliberately rebased to match #7189. Validation code should follow the #7189 direction: Cacti-owned injectable wrappers, with static facades only where legacy procedural code still needs them.

Copilot AI review requested due to automatic review settings April 28, 2026 04:44
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Symfony Validator–backed “settings constraints” pilot to centralize and enforce validation for a targeted subset of high-value settings at save time in the Cacti web UI.

Changes:

  • Introduces lib/CactiSettings::validate() to run Symfony constraints declared alongside settings definitions.
  • Adds constraints declarations to selected entries in include/global_settings.php and validates in settings.php before any DB writes.
  • Adds unit tests for validation behavior plus a short migration/how-to doc.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
composer.json Adds symfony/validator dependency.
lib/CactiSettings.php New validator wrapper: flattens settings definitions and returns first violation per setting.
include/global_settings.php Adds Assert constraints to selected settings definitions.
settings.php Runs validation before persisting settings; surfaces violations via raise_message().
tests/Unit/CactiSettingsTest.php Pest unit coverage for input shapes and constraint types.
docs/security/settings-validation.md Documents the new constraints pattern and how to extend it.

Comment thread include/global_settings.php
Comment thread include/global_settings.php Outdated
Comment thread include/global_settings.php
Comment thread include/global_settings.php
Comment thread include/global_settings.php Outdated
Comment thread lib/CactiSettings.php Outdated
Comment thread include/global_settings.php Outdated
Comment thread include/global_settings.php Outdated
Comment thread include/global_settings.php Outdated
Comment thread settings.php Outdated
@somethingwithproof somethingwithproof force-pushed the feat/develop-symfony-validator-settings branch from 4a6b642 to d35e9cc Compare April 28, 2026 05:06
Routes saved settings through Symfony Validator so include/global_settings.php
can declare per-setting constraints inline. The save handler runs one
validator pass and surfaces violations via the existing raise_message()
pattern, replacing scattered ad-hoc checks. Pilot covers 12 high-value
settings; the rest stay on the existing implicit form-render checks.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof force-pushed the feat/develop-symfony-validator-settings branch from d35e9cc to 293461c Compare April 28, 2026 06:09
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…declarations

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…toload)

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof added a commit to somethingwithproof/cacti that referenced this pull request May 8, 2026
Cacti Commit Audit fails on develop with 11 PHPStan errors that are
not yet in phpstan-baseline.neon. Append the matching entries so the
PR-A branch passes Run PHPStan at Level 6. Same set of entries
already exists on PR Cacti#7077; this is a transient catch-up that
upstream will absorb when those entries land on develop.

Files: aggregate_graphs.php, color_templates.php, graph_templates.php,
graphs.php, lib/html.php (3 entries).

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof added a commit to somethingwithproof/cacti that referenced this pull request May 8, 2026
Match the flat un-namespaced lib/CactiX.php convention used by Cacti#7088,
Cacti#7073, Cacti#7077, Cacti#7075. Drop symfony/process (added by Cacti#7073) and
symfony/validator (added by Cacti#7077) to avoid composer.json conflicts at
merge time.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof added a commit to somethingwithproof/cacti that referenced this pull request May 8, 2026
Match the un-namespaced lib/CactiX.php convention used by canonical PRs
(Cacti#7088 CactiProcess, Cacti#7073 CactiMime, Cacti#7077 CactiSettings, Cacti#7075
CactiApplication/CactiCommand). The previous nested PSR-4 namespace
required composer autoload changes that conflicted with those PRs.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof added a commit to somethingwithproof/cacti that referenced this pull request May 8, 2026
Rename lib/Security/CactiOAuth.php to lib/CactiOAuth.php and drop the
Cacti\Security namespace. Update the two callers (oauth2.php and
lib/functions.php mailer) to use the unqualified class name. Behavior
is unchanged: only the FQCN was rewritten.

The flat layout matches every other lib/Cacti*.php class on develop and
in the canonical PRs (Cacti#7088, Cacti#7073, Cacti#7077, Cacti#7075).

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Queue note: this should follow #7125. After #7125 lands, rebase this settings validation pilot onto the shared CactiValidator helper/dependency and remove duplicate composer/baseline churn.

TheWitness added a commit that referenced this pull request May 12, 2026
…7124)

* test: shared hand-off + mutation infrastructure for feature PRs

Consolidates infection/infection dev dep, allow-plugins entry, baseline
infection.json5, tests/HandOff/HandOffHelpers.php with reusable stubs
(cacti_log capture, temp-file fixtures, minimal-zip builder), HandOff
testsuite registration, and the documented pattern. Feature PRs that
add hand-off coverage rebase onto this and contribute only ONE
HandOffTest file plus an optional infection.json5 filter override.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* test: consolidate per-feature hand-off suites into shared infra

Each test guards on its feature file and skips when not present on develop.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix(test): replace eval stub, unify log buffer ref, guard ZipArchive add

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix(ci): correct infection vendor path in infection.json5

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* ci(phpstan): baseline upstream undefined-variable and isset errors

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* test: add HandOff and Integration testsuites; add regression guards

- phpunit.xml: add Integration testsuite alongside existing Unit/HandOff
- tests/Integration/: DbDumpIntegrationTest, PingIntegrationTest (from develop), CactiProcessIntegrationTest, SqlScriptsIntegrationTest
- tests/HandOff/RegressionGuardTest: guards against backsliding on shell_exec, password-in-argv, RLIKE concat, and open-redirect patterns

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* chore(test): pest test infrastructure and PSR-4 autoload

Add Pest 2 dev dependency, regenerate composer.lock under PHP 8.1, declare PSR-4 autoload for the Cacti namespace, and seed the test bootstrap with three unit tests plus an orb integration check. Pest stays on the v2 line because v3 transitively requires PHP 8.2 and breaks the 8.1 matrix entry.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* chore(phpstan): catch up baseline with 11 upstream-detected entries

Cacti Commit Audit fails on develop with 11 PHPStan errors that are
not yet in phpstan-baseline.neon. Append the matching entries so the
PR-A branch passes Run PHPStan at Level 6. Same set of entries
already exists on PR #7077; this is a transient catch-up that
upstream will absorb when those entries land on develop.

Files: aggregate_graphs.php, color_templates.php, graph_templates.php,
graphs.php, lib/html.php (3 entries).

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* chore(test): drop composer.lock; let CI resolve per PHP version

The lock file pinned brianium/paratest v7.3.1 which only supports
PHP 8.1-8.3, so the 8.4 matrix entry rejected the lock with
"Your lock file does not contain a compatible set of packages".
A single lock cannot satisfy 8.1 (paratest 7.3.x) and 8.4 (paratest
7.4.x) at the same time because pestphp/pest 2.36.0 is the last
2.x release that supports 8.1 and the 8.1-compatible paratest tag
predates 8.4 support.

Match upstream develop's behaviour: no committed lock; let
`composer install` in CI resolve per matrix entry. Pest stays on
the v2 line in composer.json so 8.1 still gets a compatible Pest.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix(test): apply Copilot review feedback for test infra

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* chore(test): drop psr-4 autoload and align deps with canonical PRs

Match the flat un-namespaced lib/CactiX.php convention used by #7088,
#7073, #7077, #7075. Drop symfony/process (added by #7073) and
symfony/validator (added by #7077) to avoid composer.json conflicts at
merge time.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* test: skip integration suites and regression guards pending feature PRs

Guards integration tests and regression guards from #7083 against
absence of CactiProcess and other feature-PR hardening, so the
consolidated foundation PR is green on develop. Suites activate
automatically once their feature PRs merge.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix(ci): drop infection/infection to unbreak PHP 8.4 matrix

infection/infection ^0.27 transitively pins thecodingmachine/safe
^2.1.2, resolving to v2.5.0. Its generated stubs use implicit-nullable
parameter declarations, which PHP 8.4 emits as deprecations into
cacti.log and trips the log-quiet assertion. infection has no config
or test wiring in this repo, so removing the dev dep clears the
transitive pin without losing functionality.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
TheWitness added a commit that referenced this pull request May 13, 2026
* refactor(oauth): extract Cacti\Security\CactiOAuth provider factory

Replace the inline provider switch in oauth2.php and the OAuth
branch of mailer() in lib/functions.php with two helpers:

- CactiOAuth::getProvider($name, $params) returns the configured
  provider instance for `google`, `azure`, `yahoo`, or `microsoft`,
  or null when the configured provider is unknown.
- CactiOAuth::getDefaultOptions($name) returns the scope set each
  provider expects.

Behaviour is preserved; both call sites now branch on a null
return rather than relying on a `$provider` variable that may have
fallen through the switch unset.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* chore(phpstan): catch up baseline with 11 upstream-detected entries

Same as PR-A: appends 11 PHPStan ignoreErrors entries that exist on
upstream develop but are not yet baselined, so this branch's CI does
not regress on phpstan analyse --level 6.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* refactor: flatten CactiOAuth to global namespace

Rename lib/Security/CactiOAuth.php to lib/CactiOAuth.php and drop the
Cacti\Security namespace. Update the two callers (oauth2.php and
lib/functions.php mailer) to use the unqualified class name. Behavior
is unchanged: only the FQCN was rewritten.

The flat layout matches every other lib/Cacti*.php class on develop and
in the canonical PRs (#7088, #7073, #7077, #7075).

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* test: add shared unit stubs for OAuth tests

* fix(oauth): load provider factory in runtime paths

* test(oauth): pin provider factory runtime loading

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: TheWitness <thewitness@cacti.net>
Default Symfony NotBlank/Length/Range/Choice/Positive messages render
in English regardless of Cacti locale because the validator runs
without a Symfony Translator. Pass explicit __()-wrapped message,
minMessage, maxMessage, and notInRangeMessage options on each bare
constraint in include/global_settings.php so violations participate
in Cacti's gettext pipeline. Documents the no-Translator decision in
CactiValidator so future contributors know to translate at the call
site.

Addresses Copilot PR Cacti#7077 review feedback.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Addressing all 11 Copilot suggestions:

  • Autoload order: global_settings.php already guards with file_exists + require_once vendor/autoload.php before the Assert use statement (lines 29-31). No change needed.
  • i18n: All constraint messages already use __() — grep for message: ' shows no unwrapped strings.
  • Choice duplication: Already uses array_keys($GLOBALS['poller_intervals']) / array_keys($GLOBALS['cron_intervals']) — no hard-coded list.
  • Redundant test require: The require at line 15 in CactiSettingsTest.php is lib/CactiSettings.php (the class under test), not vendor/autoload.php — no change needed. CactiValidatorTest.php has a genuinely redundant require, will fix there.
  • Translator for Validator: Noted for future enhancement; constraint messages explicitly pass translated strings via __() so the fallback English messages from Symfony are not reached.
  • settings.php $_POST: Will switch to gnrv() map in a follow-up commit.

# Conflicts:
#	composer.json
@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Closing as superseded by #7189, which carries this work forward as the DI-friendly component baseline. Leaving the branch in place so the review history stays accessible.

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.

2 participants