Skip to content

chore(test): gate test-only DB short-circuit behind CACTI_TEST_BOOTSTRAP env#7128

Open
somethingwithproof wants to merge 5 commits into
Cacti:developfrom
somethingwithproof:chore/test-db-sentinel
Open

chore(test): gate test-only DB short-circuit behind CACTI_TEST_BOOTSTRAP env#7128
somethingwithproof wants to merge 5 commits into
Cacti:developfrom
somethingwithproof:chore/test-db-sentinel

Conversation

@somethingwithproof

Copy link
Copy Markdown
Contributor

The existing !defined('PHP_TESTING') guards in include/global.php
skipped db_connect_real whenever PHP_TESTING was defined anywhere
in the include chain. A stray define from a CLI tool or third-party
script could disable real DB connection logic in a deployed
environment.

This change tightens the guard:

  • Require both defined('PHP_TESTING') and getenv('CACTI_TEST_BOOTSTRAP') === '1'
    before short-circuiting the DB connect calls.
  • Replace the silent no-connect branch with new Cacti_TestDbSentinel(),
    a final class whose __call throws RuntimeException for every
    invocation. Any code path that treats the sentinel as a real handle
    fails loudly instead of silently returning the wrong value.
  • Skip the remote-override block when $remote_db_cnn_id is the
    sentinel.

Self-contained: only include/global.php changes. No public API
shifts.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens Cacti’s bootstrap DB connection logic in include/global.php by preventing a stray PHP_TESTING define from accidentally disabling real database connections in non-test environments. It introduces an explicit opt-in via CACTI_TEST_BOOTSTRAP=1 and replaces silent “no-connect” behavior with a sentinel object intended to fail loudly if misused as a DB handle.

Changes:

  • Gate the test-only DB connection short-circuit behind both PHP_TESTING and CACTI_TEST_BOOTSTRAP=1.
  • Introduce Cacti_TestDbSentinel whose method calls throw to avoid silently treating a non-connection as a real handle.
  • Avoid applying the remote-override configuration when the “remote connection” is the sentinel.

Comment thread include/global.php Outdated
Comment thread include/global.php Outdated
Comment thread include/global.php
Comment thread include/global.php Outdated
…RAP env

The previous `!defined('PHP_TESTING')` guard skipped db_connect_real
whenever PHP_TESTING was set anywhere in the include chain. A stray
define from a CLI tool or third-party script could disable real DB
connection logic in a deployed environment.

Switch the guard to require both `PHP_TESTING` and an explicit
`CACTI_TEST_BOOTSTRAP=1` env var, and replace the silent-no-connect
path with a `Cacti_TestDbSentinel` whose every method throws. Any
production code path that accidentally treats the sentinel as a real
DB handle will fail loudly instead of silently misbehaving.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The previous review fix introduced a local $is_test_bootstrap flag but left
two call sites still inlining defined('PHP_TESTING') && getenv(...). Replace
the long-form check and guard the single-poller cacti_db_version fetch so the
sentinel branch finishes initialising without invoking the DB.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…sed on PHP_TESTING alone

Set the env var where PHP_TESTING is defined so legitimate test runs short-circuit the DB connect again, gate the config.php.dist swap and schema-validation skip on the combined predicate, read it with local_only, and always seed cacti_db_version.

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.

2 participants