Skip to content

refactor(symfony): align components with DI-friendly wrappers#7189

Draft
somethingwithproof wants to merge 53 commits into
Cacti:developfrom
somethingwithproof:feat/develop-symfony-di-direction
Draft

refactor(symfony): align components with DI-friendly wrappers#7189
somethingwithproof wants to merge 53 commits into
Cacti:developfrom
somethingwithproof:feat/develop-symfony-di-direction

Conversation

@somethingwithproof

@somethingwithproof somethingwithproof commented May 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds DI-friendly Cacti wrappers around Symfony Process, Console, Validator, Mime, Filesystem, Form, and Clock, with injectable instance APIs plus static facades for legacy call sites.
  • Expands CactiProcess across production shell execution paths so commands use argv arrays where practical and centralize legacy command-line execution behind one wrapper.
  • Centralizes long-lived process pipes in CactiProcessPipe, leaving production proc_open() usage in one reviewed abstraction.
  • Applies a focused CactiFilesystem pass in package, plugin, spikekill, and poller cache file operations.
  • Adds FormatterHelper access through the console bootstrap and CactiCommand.

Architecture Direction

This is the Symfony component baseline for develop. We are not introducing the full Symfony container yet. New code should depend on Cacti-owned injectable wrappers, while older procedural code can use the static facades during migration.

Process execution should prefer argv arrays through CactiProcess; runCommandLine() is reserved for internally built legacy command strings that still need compatibility. Filesystem-sensitive writes, deletes, copies, mkdirs, temp names, and path checks should use CactiFilesystem when touched.

Consolidation

This PR supersedes the direction of the smaller Symfony component PRs #7073, #7075, and #7077. Those PRs can remain useful as review history, but the merge direction should be this consolidated DI-friendly implementation.

Symfony Mailer replacement remains separate in #7190 so mail transport behavior can be reviewed independently.

Validation

  • php -l over touched PHP files
  • git diff --check
  • Source scan confirms no production exec, shell_exec, system, or passthru call sites remain
  • Raw production proc_open() is centralized in lib/CactiProcessPipe.php
  • Pest was not run locally because include/vendor/bin/pest is missing in this worktree

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>
Routes external commands through Symfony Process in array-mode argv,
scrubs the environment to a baseline allowlist, and centralizes timeout
and exit-code handling. Migrates cli/push_out_hosts.php as the pilot
call site. Includes unit, integration, and a migration guide.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Centralizes argv parsing through Symfony Console's Command/Application
classes. cmd_realtime.php and cli/splice_rrd.php become thin shims
delegating to CmdRealtimeCommand and SpliceRrdCommand, preserving the
operator-facing argv interface used by poller_realtime.php and existing
cron jobs. Argument validation goes through filter_var so values like
"3.14" are rejected instead of silently truncated; splice_rrd
filenames are checked for shell metacharacters before any processing.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
11 errors surfaced after PHPStan re-analyzed the tree on the latest
PR runs. None are in code this PR touches; they are pre-existing
issues in aggregate_graphs.php, color_templates.php, graph_templates.php,
graphs.php, and lib/html.php. Suppress them in the baseline so this
PR can land. A separate cleanup PR on develop should retire entries.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The Pest assertion was wrapped in double quotes, so PHP interpolated
$poller_id to the empty string. The actual assertion looked for the
literal $poller_id text in poller_realtime.php; switching to single
quotes preserves the dollar sign verbatim.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Pest tests pin every data boundary the wrapper crosses (argv→child,
env scrub, stdout bytes, exit code, exec-shape parity, timeout chaining).
infection.json5 scopes mutation analysis to the wrapper files with a
70% MSI floor.

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>
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>
Source-scan asserts the production branch and the bypass gate so the
escape hatch can't silently become the only path.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
PHP refuses to start without SYSTEMROOT; proc_open relies on COMSPEC and
PATHEXT to resolve executables under cmd.exe.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
A web request with a leaked CACTI_PHP_TESTING env var could otherwise
skip cli_check.php. PHP_SAPI gate eliminates that path; strict '1'
equality drops the truthy-string surface.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Routes uploaded-file MIME detection through Symfony Mime so a content-type
allowlist can be enforced on top of the existing extension and signature
checks. package_import.php now rejects uploads whose detected MIME is not
in the allowlist (zip, gzip, xml variants) before any further processing.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
LIBXML_NONET blocks network entity loading as defense in depth even
though libxml2 ≥ 2.9.0 disables external entities by default.

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>
…t timeout to float

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

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Allows callers to inject env name=value pairs without putenv() or argv exposure.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
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>
- Add explicit $onError callback to forward stderr chunks to STDERR,
  replacing reliance on CactiProcess's internal fallback.
- Capture runStreaming() return value and pass to exit() so the wrapper
  propagates the child's exit status rather than always exiting 0.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
# Conflicts:
#	composer.json
#	graph_realtime.php
#	tests/Unit/GraphRealtimeShellTest.php
#	tests/integration/CactiProcessIntegrationTest.php
# Conflicts:
#	cmd_realtime.php
#	composer.json
#	phpunit.xml
#	tests/Unit/GraphRealtimeShellTest.php
# Conflicts:
#	composer.json
…ony-di-direction

# Conflicts:
#	composer.json
@somethingwithproof

Copy link
Copy Markdown
Contributor Author

Added a follow-up process hardening commit (2eb5ec4) on this branch.

Summary:

  • Replaced production exec() / shell_exec() call sites with CactiProcess argv-mode calls where the process runs to completion.
  • Kept long-lived bidirectional proc_open() sessions for the script server and rrdtool pipe, but changed those command arguments to argv arrays so PHP does not route through a shell string.
  • Moved mysql password handling for mysqladmin/mysql schema load to MYSQL_PWD via env_set instead of argv strings.
  • Added a source guard test covering the migrated production files.

Local validation:

  • php -l over all touched PHP files
  • git diff --check
  • Source scan: no production exec(), shell_exec(), system(), or passthru() calls remain outside tests/comments after the update.

@somethingwithproof

Copy link
Copy Markdown
Contributor Author

Added follow-up commit 46a0579.

Summary:

  • Introduces CactiProcessPipe as the only production wrapper around proc_open for long-lived bidirectional subprocesses.
  • Moves cmd.php, remote_agent.php, and cli/rrdresize.php pipe launches through CactiProcessPipe.
  • Extends guard coverage so raw production proc_open is only allowed in CactiProcessPipe.
  • Applies a focused CactiFilesystem pass across plugin archive restore/archive, package temp XML cleanup, spikekill XML/backup handling, and poller cache write paths.
  • Moves the poller cache PHP syntax check from system() to CactiProcess.

Validation:

  • php -l over touched files
  • git diff --check
  • Source scans remain clean for production exec/shell_exec/system/passthru. Raw proc_open is now centralized in lib/CactiProcessPipe.php.

@somethingwithproof somethingwithproof force-pushed the feat/develop-symfony-di-direction branch from 01036a5 to d4d60d8 Compare June 8, 2026 06:40
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
… fatal

- tests/HandOff/CliInvocationHandOffTest.php declares its own runCli(), and the duplicate definition aborted the whole Pest run with a silent exit 255

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

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

- template/package import allowlists accept libmagic text/plain; spikekill writeXMLFile and poller resource_cache_out catch wrapper exceptions and fall back to the boolean failure path; migration guard test scoped to migrated callers

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof somethingwithproof changed the title [codex] Align Symfony components with DI-friendly wrappers refactor(symfony): align components with DI-friendly wrappers Jun 14, 2026
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Clears the level 6 alreadyNarrowedType error on lib/data_query.php:2591 that
develop currently emits, matching the sibling narrowing identifiers already ignored.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
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