Tags: UltraBob/ddev-drupal-code-quality
Tags
Node tooling at project root, installer UX improvements, scan scope f… …ixes (#32) * Add shared root-only Node toolchain resolver Introduce commands/helpers/node-toolchain.sh with resolve_node_tool(), a single function that looks up Node tool binaries exclusively in the project root node_modules. All Node wrappers will source this helper instead of duplicating resolution logic. Register the helper in install.yaml so DDEV copies it into .ddev/commands/helpers/ on add-on install. * Route stylelint wrappers through shared Node resolver Replace CORE_STYLELINT/ROOT_STYLELINT variables and the if/elif/else fallback chain with resolve_node_tool(). Remove the core/node_modules tar exclude from stylelint-fix preview mode, replacing it with a generic docroot-relative nested node_modules exclude. Update error messages to reference the project root and the installer. * Route eslint wrappers through shared Node resolver Replace CORE_ESLINT/ROOT_ESLINT, ESLINT_TOOLCHAIN/prefer_root logic, and the core fallback in resolve_plugins_dir() with resolve_node_tool(). Remove the core/node_modules tar exclude from eslint-fix preview mode, replacing it with a generic docroot-relative nested exclude. Update error messages to reference the project root and the installer. * Route prettier wrappers through shared Node resolver Replace CORE_PRETTIER/ROOT_PRETTIER variables and the core fallback chain with resolve_node_tool(). Rename PRETTIER_BIN to TOOLCHAIN_BIN for consistency with the helper contract. Remove the core/node_modules tar exclude from prettier-fix preview mode, replacing it with a generic docroot-relative nested exclude. Update error messages to reference the project root and the installer. * Route cspell wrappers through shared Node resolver Replace CORE_CSPELL/ROOT_CSPELL variables and the core fallback chain with resolve_node_tool(). Rename CSPELL_BIN to TOOLCHAIN_BIN for consistency with the helper contract. Update error messages to reference the project root and the installer. * Remove core/node_modules fallback from installer and tests Strip the two core/node_modules paths from node_toolchain_present() so it only checks the project root. Add legacy core-only detection: when the root toolchain is absent but core has eslint, non-interactive mode auto-installs at root and interactive mode falls through to the prompt. Rename DOCROOT_CORE to DOCROOT_COREDIR in the package.json derivation helpers to avoid false positives on the DoD grep gate (the variable points at Drupal's core/ source dir, not node_modules). In tests: remove the web/core/node_modules early-return from ensure_node_toolchain(), add an ESLint sentinel to the fake-core- binaries regression fixture, and add an eslint --version assertion. Update README to remove stale ESLINT_TOOLCHAIN=core/auto/root docs and note the root-only toolchain policy. * Simplify stylelint wrappers by delegating config and ignore handling to stylelint Rewrite both stylelint and stylelint-fix as thin delegating wrappers (~90 lines each, down from 400+). Native stylelint behavior now handles config discovery (cosmiconfig walk), .stylelintignore auto-application, plugin resolution, and SCSS detection — all previously reimplemented in the wrapper scripts. Wrapper responsibilities are now limited to: toolchain resolution via the shared helper, host-path translation (map_path for IDE integration), default-glob injection on bare invocation, and cd to PROJECT_ROOT. This eliminates the CMD-array-reassignment bug class (issue #27) by construction and fixes an unreported bug where stylelint.config.js configs used by modern Drupal themes were silently skipped. The --preview mode in stylelint-fix is removed; a deprecation warning directs users to git diff as the standard review workflow. * Narrow default scan to CSS and add SCSS setup hint Default globs now only include **/*.css — the base config shipped by DCQ is CSS-only, so including SCSS/Sass by default would cause parse errors on projects that haven't opted into SCSS linting. When SCSS/Sass files are detected during a bare invocation, a one-line hint is emitted to stderr suggesting the stylelint-config-standard-scss setup recipe. Users can still lint SCSS explicitly via glob arguments. Also hardens --preview removal: the flag now exits with an error instead of falling through to --fix. * Fix SCSS hint to detect .scss files and skip dependency directories The find expression had an operator precedence bug that only matched .sass files (not .scss) due to implicit -print behavior with -o. Also prune node_modules and vendor to avoid spurious hints triggered by SCSS files in dependencies rather than user code. * Update bats tests for minimal stylelint wrapper contract Rewrite stylelint-related tests to assert the new wrapper behavior: native config discovery and ignore-file handling instead of wrapper- injected --config, --config-basedir, and --ignore-path flags. - Merge two near-identical config-delegation tests into one - Rewrite ignore-path regression tests (issue #27) to verify the flag injection mechanism no longer exists, making the bug structurally impossible - Add new test for SCSS hint on default invocation - Strengthen non-web docroot test with echo stub to verify path pass-through - Update full-test preview section for --preview removal (exit 1) 37/37 pass (35 active, 2 skipped for DCQ_FULL_TESTS). Full-test suite: 2/2 pass (fresh install + preview mode). * Fix escaped command display in installer run_command output The printf %q format was shell-escaping arguments in the "Running:" line, producing unreadable output like "cd\ /var/www/html\ \&\&\ npm\ install". Use %s instead to show commands as the user would type them. * Improve SCSS hint wording and fix escaped installer output - SCSS hint now shows how to add SCSS to default runs rather than suggesting explicit paths each time - Fix printf %q escaping in run_command that produced unreadable "Running:" lines in installer output * Include SCSS and Sass in default stylelint scan globs Default globs now include **/*.scss and **/*.sass alongside **/*.css, so bare `ddev stylelint` scans all style files. Users who configure SCSS support (e.g. extending stylelint-config-standard-scss) no longer need to pass explicit globs on every invocation. Removes the SCSS hint and its find-based detection logic. * Add dist, build, and playwright-report to default ignore lists Generated output directories cause false positives and slow scans. Adds **/dist/** and **/build/** as active exclusions and **/playwright-report/** as a commented-out option with a note. * Default stylelint globs to CSS-only with configurable override The shipped .stylelintrc.json only supports CSS, so default scan globs now match only **/*.css. Users can override via DCQ_STYLELINT_GLOBS in web_environment to add SCSS/Sass/other patterns. Ships a new config.drupal-code-quality.yaml with the commented default. * Add SCSS detection and guided setup to installer Detects .scss/.sass files during install and offers interactive SCSS support setup. When accepted, writes SCSS globs to the add-on config and shows remaining manual steps (package install, config edit) in the install summary. Non-interactive installs print full guidance. Checks both stylelint config and DDEV env to avoid re-prompting when SCSS is already fully configured. * Fix IDE settings merge losing Python exit codes The merge_json_settings and merge_json_extensions functions used `if ! python ...; then return 1; fi` which always returned 1 for any non-zero Python exit, discarding the actual exit code. This meant exit 3 (no changes needed) was misreported as a merge error, causing "Unable to merge IDE settings/extensions" on projects where settings were already up to date. Propagate the real exit code with `return $?`. * Fix SCSS detection to check nested configs and respect recommended mode SCSS detection now searches all .stylelintrc* files up to 4 levels deep instead of only the project root, so themes with their own SCSS config are properly detected as configured. Also gates the interactive SCSS prompt behind recommended_mode=0 so it doesn't appear when the user accepts recommended defaults. * Handle composer require failure gracefully with retry/proceed/abort When composer require fails during Phase 1 (e.g., security advisories blocking resolution, allow-plugins not configured), the installer now catches the failure instead of aborting. Users see the full composer error output plus common causes, and get three options: - retry (fix the issue in another terminal, then try again) - proceed (continue with remaining phases without PHP dev tools) - abort (stop the installer entirely) In non-interactive mode, the installer auto-proceeds. The install summary reflects the failure with a FIX action item in next steps. Validated against all 11 projects that failed in UAT (24% of tested projects). All now complete installation successfully. * Add Node.js version check to prevent stylelint crashes on old Node Stylelint crashes with a cryptic TypeError (findLastIndex) on Node.js 16, which was found on 2 of 46 UAT projects (ccet, kcom3). Both had nodejs_version: "16" in their DDEV config and no engines field in package.json. Installer changes: - Detect nodejs_version from .ddev/config.yaml before Phase 3 - Prompt abort/skip when Node < 18 with actionable fix commands - Offer to add engines.node >= 20.0 to existing package.json Runtime wrapper changes: - Add Node >= 18 check to stylelint and stylelint-fix wrappers - Clear error message instead of cryptic findLastIndex crash Documentation: - Add Troubleshooting section to README covering the findLastIndex crash and the string formatter RangeError (known stylelint wontfix, workaround: --formatter compact) Tests: - Wrapper rejects Node < 18 with clear error - Installer prompts abort/skip on old Node - Installer offers engines.node for package.json without it * Add curated DCQ package list and upstream drift detection Introduce dcq-packages.json with the specific npm packages DCQ configs require, replacing the previous approach of copying all core devDependencies. The installer now resolves only these packages (with versions from core) when creating project-root package.json files. Add package list drift check to sync-upstream-configs.sh so curated packages are validated against core on each sync. * Exclude DDEV-generated files and playwright-report from default scans Suppress false positives on fresh installs by excluding files that are not user-authored code: - settings.ddev.php excluded from PHPCS, PHPStan, and CSpell - default.settings.php excluded from PHPStan (template, not user-edited) - playwright-report/ excluded from all JS/CSS linters and CSpell (generated build output with minified code) * Add stylelint config amendment with allowEmptyInput Create .stylelintrc.dcq.json amendment file that adds allowEmptyInput: true to the stylelint config during installation. This prevents stylelint from failing with AllFilesIgnoredError on projects with no custom CSS files (e.g., a fresh Drupal install). Uses the config-amendments pattern consistent with phpstan.dcq.neon and .phpcs.dcq.xml, keeping the upstream .stylelintrc.json untouched. * Improve installer UX: accurate summary, reduced noise, config amendments Address findings from UX audit of the installer: - Summary reports actual tool state ("already present (skipped)") instead of claiming "NOT installed" when tools exist from a prior install - Phase 1 emits "PHP dev tools already present" when nothing to install - Suppress "Removed dcq-install.sh" implementation detail from output - SCSS setup steps use letters (a-d) to avoid confusion with numbered next-steps list - Add stylelint config merge via .stylelintrc.dcq.json amendment - Exclude dcq-packages.json from project root copy (internal manifest) - Clean up stale dcq-packages.json from prior installs - Add "this may take several minutes" to npm install message - Use ddev npm/yarn wrappers instead of ddev exec in install commands - Use curated dcq-packages.json for dependency resolution * Fix stylelint formatter crash on large projects Use compact formatter for stylelint in ddev checks to prevent RangeError crash when the table formatter tries to render thousands of violations. The standalone ddev stylelint command keeps the default table formatter for smaller projects. Also fix the stylelint wrapper argument parser to properly handle flags that take a value argument (--formatter, --custom-syntax, etc.) without treating the value as a positional file path. * Add tests for UX audit fixes and update stub package.json Add test coverage for: - dcq-packages.json not copied to project root - allowEmptyInput merged into .stylelintrc.json - settings.ddev.php excluded from PHPCS, PHPStan, CSpell - playwright-report excluded from ignore files - Summary says "already present" on re-install - No "Removed dcq-install.sh" in output Update stub package.json fixtures to include the full curated DCQ package list matching dcq-packages.json. * Exclude config/sync from CSpell and scope theme scanning to custom only Address issue #30: default scans produce false positives from non-user-authored content. - CSpell: add config/sync to ignorePaths. Exported Drupal config YAML contains UUIDs and hash tokens that are not misspelled words. - All tools: only scan themes/custom/, excluding non-custom themes like blank/ (ships with Drupal CMS outside contrib/). Applied consistently: - PHPCS: explicit <file> for themes/custom - PHPStan: already scoped to themes/custom - Stylelint, ESLint, Prettier: ignore themes/** then un-ignore themes/custom/** - CSpell: ignore themes/!(custom)/** * Sync upstream CSpell words and fix Mutagen sync race in tests - Apply upstream prepare-cspell.php change: add 'flagwords' and 'mkdocs' to CSpell word list (closes #28) - Fix wait_for_container_path to flush Mutagen sync and check file has content (test -s) not just existence (test -r). Under high system load, file inodes appear before content syncs, causing ESLint and other tools to see empty files and report no errors. * Fix theme ignore negation pattern across all tools The previous pattern (**/themes/** + !**/themes/custom/**) did not work in ESLint 8 or stylelint — the negation failed to un-ignore files, causing custom theme code to be silently skipped. Switch to **/themes/*/ + !**/themes/custom/ which ignores directory entries one level under themes/ except custom/. Verified working in ESLint 8, stylelint, and prettier. Also remove debug instrumentation from test file. * Fix test assertions for PHPCS scope change and new installer behavior - Test 10: create fake vendor/bin tools so "already present" check works in test environments without real composer installs - Test 11: narrow refute_output to "Removed" only — "dcq-install.sh" appears in DDEV's own file copy output - Test 20: update docroot assertions for new PHPCS scan paths (modules, themes/custom, profiles, sites instead of whole docroot)
Upstream config changes (#29) * Set PHPCS default scope to docroot and test docroot merge * Add upstream config sync script, detection accuracy tests, and volume cleanup Add a maintainer script that fetches the latest config files from Drupal core and GitLab CI templates, compares them against local assets, and optionally applies updates. A weekly GitHub Actions workflow detects upstream drift and opens an issue when changes are found. Expand the full install test with detection accuracy assertions for every wrapper command: true positives (tools catch known violations), true negatives (clean code passes), and exclusion path tests (contrib code is not flagged). Clean up Docker volumes (mariadb, mutagen, snapshots) in test teardown to prevent accumulation across test runs. * Fix cspell comparison to ignore arrays populated by prepare-cspell.php The local .cspell.json asset has populated ignorePaths, dictionaries, dictionaryDefinitions, and words arrays (expanded by prepare-cspell.php at install time). Upstream has empty arrays. Strip these from both sides before comparing so the script only flags structural changes to fields like description, flagWords, and overrides. Also revert accidental asset updates from test run. * Update upstream config * Ensure trailing newlines on assets for DDEV addon checker The DDEV addon update checker requires files to end with a newline, but some upstream files lack one. Add trailing newline to .eslintrc.jquery.json and normalize fetched files in the sync script so the difference doesn't cause false positives.
fix: handle npm install failures gracefully instead of crashing insta… …ller (#24) The installer crashed with exit status 1 when npm install failed during Phase 3, because unguarded run_command calls allowed set -e to abort the entire post-install action. Changes: - Wrap all npm/yarn install calls in error handlers so failures produce helpful retry messages instead of crashing the installer - Replace mapfile with a bash 3.2-compatible while-read loop (macOS ships bash 3.2 which lacks mapfile, causing an unbound variable crash) - Use test -s instead of test -f when waiting for .cspell.json in the container, so the wait detects empty files from partial filesystem sync - Only emit "Node toolchain installed" when installation actually succeeds Fixes #23
Improve cspell expansion execution and prettier-fix Sass scope (#5) * Remove hidden --quiet flag from eslint wrapper * Remove hidden --quiet flag from eslint-fix wrapper * Make ESLint quiet default explicit and IDE-aligned * Clarify ESLint quiet config for CLI and VS Code * Improve cspell expansion execution and prettier-fix sass scope
Add maintenance checker CI gate and mutagen sync metadata
Fix test assertions for preview mode after fix command refactor The recent refactor of fix commands removed the summary output and the test update commits missed correcting two assertions: - Changed assertion from removed "ESLint-fix summary:" to actual "Running ESLint to report any remaining issues" output - Fixed patch filename assertions to use correct names (_eslint.patch, _prettier.patch, _stylelint.patch) instead of incorrect names that were never generated by the commands
PreviousNext