Skip to content

Fixing pre-commit hooks#6891

Merged
TheWitness merged 2 commits intoCacti:developfrom
TheWitness:pre-commit-changes
Mar 27, 2026
Merged

Fixing pre-commit hooks#6891
TheWitness merged 2 commits intoCacti:developfrom
TheWitness:pre-commit-changes

Conversation

@TheWitness
Copy link
Copy Markdown
Member

This change relocates the pre-commit-check.sh script and renames some composer run-script names.

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

This PR updates the repo’s pre-commit workflow by relocating the hook wrapper script and renaming Composer script entries used by the hook to run linting/analysis.

Changes:

  • Point pre-commit hook entries to tests/tools/pre-commit-check.sh.
  • Update tests/tools/pre-commit-check.sh preflight/tool invocation logic.
  • Rename Composer scripts from phpcsfixer/phpcsfixit to php-cs-fixer/php-cs-fixit.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/tools/pre-commit-check.sh Updates pre-commit wrapper logic and Composer script names.
composer.json Renames Composer scripts for PHP CS Fixer.
.pre-commit-config.yaml Re-points local hooks to the relocated script.
Comments suppressed due to low confidence (4)

tests/tools/pre-commit-check.sh:101

  • The tool load check is broken: [ $($VENDOR_BIN/$tool --version > /dev/null 2>&1) -gt 0 ] uses command substitution (capturing stdout) and then performs a numeric comparison. Because stdout is redirected to /dev/null, the substitution expands to an empty string and the test errors (and with set -e will abort the hook). Use a direct command-status check instead (e.g., negate the --version invocation) rather than trying to compare output to a number.
    tests/tools/pre-commit-check.sh:115
  • The phplint availability check is incorrect for the same reason as check_tool: [ $($VENDOR_BIN/phplint --version > /dev/null 2>&1) -gt 0 ] evaluates to an empty string and causes a failing test. This will either break the hook or always fall back unexpectedly; prefer checking the command’s exit status directly.
    tests/tools/pre-commit-check.sh:154
  • composer run-script php-cs-fixer is being executed inside $() and then compared with -gt 0, but composer run-script prints text to stdout and doesn’t output a numeric status. With set -e, this will behave unpredictably and can fail with a "test: illegal number" error. Invoke the composer script normally and branch on its exit code instead.
    tests/tools/pre-commit-check.sh:78
  • The message has a typo/grammar issue ("your are") and extra spacing, and it’s labeled as WARNING but exits non-zero (effectively an error). Consider correcting the wording ("you are") and either change the prefix to ERROR or don’t exit if it’s intended to be only a warning.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@TheWitness TheWitness merged commit d8d5a94 into Cacti:develop Mar 27, 2026
5 checks passed
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.

4 participants