From 3452d55b4a04ae6455c017ddd0ecbadf19c81b02 Mon Sep 17 00:00:00 2001 From: Bob McDonald Date: Fri, 24 Jul 2026 13:59:52 -0700 Subject: [PATCH] Fix Node dependency install quoting and failure reporting The versioned JS dependency install always failed because name@range tokens were escaped with printf %q before being word-split into the command array: npm received a literal \^ and rejected it as an invalid dist-tag (EINVALIDTAGNAME). The failure was then masked because maybe_install_missing_root_deps returned 1 both for "nothing missing" and "install failed", so the caller fell through to a bare npm install that succeeded trivially, printed "Node toolchain installed", and listed the toolchain in the final summary. - Pass name@range tokens to npm/yarn as raw argv elements; run_command execs them without a shell reparse, so no escaping is needed. - Give maybe_install_missing_root_deps distinct return codes (installed / failed / nothing missing / skipped) and only fall through to the bare install when nothing was missing or the add was skipped. Failures now produce a "Node toolchain: FAILED" summary entry with retry instructions, matching the PHP dev-tools pattern. - Capture the return code at the existing-toolchain top-up call site too, so a failed top-up is reported as FAILED instead of the pre-existing node_modules reading as "already present". - Print versioned retry commands (constraints resolved from Drupal core) instead of the unversioned package list, which hits peer dependency conflicts (latest eslint-plugin-yml requires eslint >= 9 while eslint-config-airbnb-base requires eslint 7/8), and note the version-alignment caveat on the static fallback list. Add stub-based installer tests covering the unescaped argv path, the FAILED reporting on install failure, and the existing-toolchain top-up failure, plus real-ddev tests for the versioned dependency add and the FAILED summary. --- dcq-install.sh | 88 ++++++++---- tests/test-installer-deps-prompt.bats | 196 ++++++++++++++++++++++++++ tests/test.bats | 86 +++++++++++ 3 files changed, 346 insertions(+), 24 deletions(-) diff --git a/dcq-install.sh b/dcq-install.sh index 7dc3258..22c253d 100644 --- a/dcq-install.sh +++ b/dcq-install.sh @@ -590,6 +590,8 @@ PHP maybe_install_missing_root_deps() { # Prompt to add missing root devDependencies when root package.json exists. + # Returns: 0 = missing deps installed, 1 = install (or dep computation) + # failed, 2 = nothing missing, 3 = skipped (non-interactive or declined). local ddev_cmd="$1" local non_interactive="$2" local package_manager="$3" @@ -598,11 +600,12 @@ maybe_install_missing_root_deps() { local missing_node_deps if ! missing_node_deps="$(find_missing_node_deps "$ddev_cmd")"; then + emit 'Failed to compute missing Node dependencies.\n' return 1 fi if [ -z "$missing_node_deps" ]; then - return 1 + return 2 fi # Avoid mapfile (requires bash 4+; macOS ships bash 3.2). @@ -629,37 +632,34 @@ maybe_install_missing_root_deps() { should_add=1 elif [ "$non_interactive" -eq 1 ]; then emit 'Skipping dependency add (non-interactive). Install the missing packages to avoid lint errors.\n' - return 1 + return 3 elif prompt_yes_no "$prompt_msg" 1; then should_add=1 fi if [ "$should_add" -eq 1 ]; then - deps_cmd="" - for dep in "${missing_node_deps_array[@]}"; do - deps_cmd+=" $(printf '%q' "$dep")" - done + # Pass name@range tokens straight through as argv elements; run_command + # execs them without a shell reparse, so no escaping (npm rejects a + # literal \^ in a version range as an invalid dist-tag). if [ "$package_manager" = "npm" ]; then - # shellcheck disable=SC2086 - cmd=( "$ddev_cmd" "npm" "install" "--save-dev" "--package-lock"${deps_cmd} ) + cmd=( "$ddev_cmd" "npm" "install" "--save-dev" "--package-lock" "${missing_node_deps_array[@]}" ) if ! run_command "${cmd[@]}"; then emit 'npm install --save-dev failed. You can retry manually.\n' - emit_dcq_package_list "npm" + emit_node_install_command "npm" "$missing_node_deps" 1 return 1 fi emit 'Node dependencies added (project root).\n' cmd=( "$ddev_cmd" "npm" "install" "--package-lock" ) if ! run_command "${cmd[@]}"; then emit 'npm install failed. You can retry manually.\n' - emit_dcq_package_list "npm" + emit_node_install_command "npm" "$missing_node_deps" 1 return 1 fi else - # shellcheck disable=SC2086 - cmd=( "$ddev_cmd" "yarn" "add" "-D"${deps_cmd} ) + cmd=( "$ddev_cmd" "yarn" "add" "-D" "${missing_node_deps_array[@]}" ) if ! run_command "${cmd[@]}"; then emit 'yarn add failed. You can retry manually.\n' - emit_dcq_package_list "yarn" + emit_node_install_command "yarn" "$missing_node_deps" 1 return 1 fi emit 'Node dependencies added (project root).\n' @@ -668,7 +668,7 @@ maybe_install_missing_root_deps() { fi emit 'Skipping missing dependency install. ESLint plugins may be unavailable.\n' - return 1 + return 3 } prompt_node_install_action() { @@ -779,6 +779,8 @@ emit_dcq_package_list() { else emit ' ddev npm install --save-dev %s\n' "$packages" fi + emit 'If the install reports dependency conflicts, add the version ranges\n' + emit "from Drupal core's package.json (e.g. %s/core/package.json) to each package.\n" "${DCQ_DOCROOT:-web}" } prompt_yes_no() { @@ -2491,7 +2493,13 @@ if [ "$core_package_json_present" -eq 1 ]; then if [ "$root_pm" = "yarn" ]; then ensure_root_yarnrc "$app_root" fi - maybe_install_missing_root_deps "$ddev_cmd" "$non_interactive" "$root_pm" 0 0 || true + existing_deps_status=0 + maybe_install_missing_root_deps "$ddev_cmd" "$non_interactive" "$root_pm" 0 0 || existing_deps_status=$? + if [ "$existing_deps_status" -eq 1 ]; then + # Surface the failed top-up in the summary instead of letting the + # pre-existing node_modules read as "already present". + node_deps_failed=1 + fi fi fi if [ "$node_mode_raw" = "skip" ]; then @@ -2625,17 +2633,24 @@ if [ "$core_package_json_present" -eq 1 ]; then fi node_install_done=0 + node_install_failed=0 if [ "$target" = "root" ]; then if [ "$has_root_package_json" -eq 1 ]; then root_pm="$(detect_package_manager "$app_root")" if [ "$root_pm" = "yarn" ]; then ensure_root_yarnrc "$app_root" fi - if maybe_install_missing_root_deps "$ddev_cmd" "$non_interactive" "$root_pm" "$root_auto_add" "$root_suppress_list"; then - node_install_done=1 - fi + # 0 = installed, 1 = failed, 2 = nothing missing, 3 = skipped. + # 2 and 3 fall through to the bare install below, which only + # materializes dependencies already declared in package.json. + deps_status=0 + maybe_install_missing_root_deps "$ddev_cmd" "$non_interactive" "$root_pm" "$root_auto_add" "$root_suppress_list" || deps_status=$? + case "$deps_status" in + 0) node_install_done=1 ;; + 1) node_install_failed=1 ;; + esac fi - if [ "$node_install_done" -eq 0 ]; then + if [ "$node_install_done" -eq 0 ] && [ "$node_install_failed" -eq 0 ]; then if [ -z "${root_pm:-}" ]; then root_pm="$(detect_package_manager "$app_root")" if [ "$root_pm" = "yarn" ]; then @@ -2651,13 +2666,18 @@ if [ "$core_package_json_present" -eq 1 ]; then if run_command "${cmd[@]}"; then node_install_done=1 else + node_install_failed=1 + # The deps are already declared in package.json here, so the + # retry is a plain install — no package list needed. emit 'JS dependency install failed. You can retry manually:\n' emit ' ddev %s install\n' "${root_pm:-npm}" - emit_dcq_package_list "${root_pm:-npm}" fi fi if [ "$node_install_done" -eq 1 ]; then emit 'Node toolchain installed (project root).\n' + elif [ "$node_install_failed" -eq 1 ]; then + node_deps_failed=1 + emit 'Node toolchain install FAILED (project root). See errors above; the summary includes retry instructions.\n' fi fi fi @@ -2855,6 +2875,7 @@ print_install_summary() { local configs_count="$4" local has_scss="$5" local pkg_mgr="${6:-npm}" + local node_missing_deps="${7:-}" emit '\n' emit '===============================================================\n' @@ -2879,6 +2900,8 @@ print_install_summary() { emit ' - Node toolchain (ESLint, Prettier, Stylelint)\n' elif [ "$node_status" = "present" ]; then emit ' - Node toolchain: already present (skipped)\n' + elif [ "$node_status" = "failed" ]; then + emit ' - Node toolchain: FAILED (see warning below)\n' else emit ' - Node toolchain: NOT installed\n' fi @@ -2903,9 +2926,24 @@ print_install_summary() { step_num=$((step_num + 1)) fi - if [ "$node_status" != "root" ] && [ "$node_status" != "present" ]; then + if [ "$node_status" = "failed" ]; then + emit ' %s. FIX: Node tooling failed to install. Resolve the %s issue\n' "$step_num" "$pkg_mgr" + emit ' (see error output above), then run:\n' + if [ -n "$node_missing_deps" ]; then + emit_node_install_command "$pkg_mgr" "$node_missing_deps" 1 + else + # No versioned list means the deps were already declared and a plain + # install failed; the retry is the same plain install. + emit ' ddev %s install\n' "$pkg_mgr" + fi + step_num=$((step_num + 1)) + elif [ "$node_status" != "root" ] && [ "$node_status" != "present" ]; then emit ' %s. Install Node tools:\n' "$step_num" - emit_dcq_package_list "$pkg_mgr" + if [ -n "$node_missing_deps" ]; then + emit_node_install_command "$pkg_mgr" "$node_missing_deps" 1 + else + emit_dcq_package_list "$pkg_mgr" + fi step_num=$((step_num + 1)) fi @@ -2977,7 +3015,9 @@ if [ "$php_deps_summary" = "skipped" ]; then fi node_summary="skipped" -if [ -n "${node_target_choice:-}" ] && [ "$node_target_choice" = "root" ]; then +if [ "${node_deps_failed:-0}" -eq 1 ]; then + node_summary="failed" +elif [ -n "${node_target_choice:-}" ] && [ "$node_target_choice" = "root" ]; then node_summary="root" fi # Check if Node toolchain is actually present regardless of what happened in this run. @@ -3010,4 +3050,4 @@ if [ "$scss_detected" = "false" ] && detect_scss_files "$app_root"; then fi fi -print_install_summary "$php_deps_summary" "$node_summary" "$ide_summary" "$configs_copied" "$scss_detected" "${root_pm:-npm}" +print_install_summary "$php_deps_summary" "$node_summary" "$ide_summary" "$configs_copied" "$scss_detected" "${root_pm:-npm}" "${missing_node_deps:-}" diff --git a/tests/test-installer-deps-prompt.bats b/tests/test-installer-deps-prompt.bats index 0a0b6dc..dd1ae53 100644 --- a/tests/test-installer-deps-prompt.bats +++ b/tests/test-installer-deps-prompt.bats @@ -489,6 +489,202 @@ PY [[ "$output" == '{"local":"keep-me"}' ]] } +write_node_deps_stub() { + # ddev stub that reports missing JS deps (with semver ranges) from the + # find_missing_node_deps PHP helper and logs every invocation verbatim. + # DDEV_STUB_NPM_SAVE_DEV_EXIT controls the exit code of + # `ddev npm install --save-dev ...` (default 0). + cat > "${STUB_BIN}/ddev" <<'SH' +#!/usr/bin/env bash +set -euo pipefail + +log_file="${DDEV_STUB_LOG:?missing DDEV_STUB_LOG}" +printf '%s\n' "$*" >> "${log_file}" + +if [[ "$*" == *"php -r"* ]]; then + printf 'cspell@^9.8.0\neslint@^8.57.1\neslint-plugin-yml@^1.19.1\n' + exit 0 +fi + +if [ "${1:-}" = "npm" ] && [ "${2:-}" = "install" ] && [ "${3:-}" = "--save-dev" ]; then + if [ "${DDEV_STUB_NPM_SAVE_DEV_EXIT:-0}" -ne 0 ]; then + echo 'npm error code EINVALIDTAGNAME (stub)' >&2 + exit "${DDEV_STUB_NPM_SAVE_DEV_EXIT}" + fi + exit 0 +fi + +exit 0 +SH + chmod +x "${STUB_BIN}/ddev" +} + +write_root_package_json_without_deps() { + cat > "${APP_ROOT}/package.json" <<'JSON' +{ + "name": "dcq-root", + "private": true, + "engines": { + "node": ">= 20.0" + } +} +JSON +} + +@test "node dep install passes semver ranges to npm unescaped" { + write_node_deps_stub + write_root_package_json_without_deps + + run env \ + "PATH=${STUB_BIN}:${PATH}" \ + "DDEV_APPROOT=${APP_ROOT}" \ + "DCQ_NONINTERACTIVE=true" \ + "DCQ_INSTALL_DEPS=skip" \ + "DCQ_INSTALL_NODE_DEPS=root" \ + "DCQ_INSTALL_IDE_SETTINGS=skip" \ + "DCQ_INSTALL_GITIGNORE=skip" \ + "DCQ_PHPSTAN_LEVEL=0" \ + bash "${ADDON_ROOT}/dcq-install.sh" + [ "$status" -eq 0 ] + [[ "$output" == *"Node toolchain installed (project root)."* ]] + + # The name@range tokens must reach npm exactly as resolved from core: + # no backslash-escaped carets (npm rejects \^ as an invalid dist-tag). + run_search '^npm install --save-dev --package-lock cspell@\^9\.8\.0 eslint@\^8\.57\.1 eslint-plugin-yml@\^1\.19\.1$' "${DDEV_STUB_LOG}" + [ "$status" -eq 0 ] + run grep -F 'cspell@\^' "${DDEV_STUB_LOG}" + [ "$status" -ne 0 ] +} + +@test "failed node dep install is reported as FAILED, not success" { + write_node_deps_stub + write_root_package_json_without_deps + + run env \ + "PATH=${STUB_BIN}:${PATH}" \ + "DDEV_APPROOT=${APP_ROOT}" \ + "DDEV_STUB_NPM_SAVE_DEV_EXIT=1" \ + "DCQ_NONINTERACTIVE=true" \ + "DCQ_INSTALL_DEPS=skip" \ + "DCQ_INSTALL_NODE_DEPS=root" \ + "DCQ_INSTALL_IDE_SETTINGS=skip" \ + "DCQ_INSTALL_GITIGNORE=skip" \ + "DCQ_PHPSTAN_LEVEL=0" \ + bash "${ADDON_ROOT}/dcq-install.sh" + + # The installer finishes (failure is non-fatal, matching the PHP deps + # behavior) but must not claim the toolchain was installed. + [ "$status" -eq 0 ] + [[ "$output" == *"npm install --save-dev failed. You can retry manually."* ]] + [[ "$output" == *"Node toolchain install FAILED (project root)."* ]] + [[ "$output" == *"Node toolchain: FAILED"* ]] + [[ "$output" == *"FIX: Node tooling failed to install."* ]] + [[ "$output" != *"Node toolchain installed (project root)."* ]] + [[ "$output" != *"- Node toolchain (ESLint, Prettier, Stylelint)"* ]] + + # The retry guidance must include the versioned tokens for every missing + # package (displayed either raw or %q-escaped for copy-paste safety). + for token in 'cspell@^9.8.0' 'eslint@^8.57.1' 'eslint-plugin-yml@^1.19.1'; do + escaped="${token/@^/@\\^}" + [[ "$output" == *"$token"* || "$output" == *"$escaped"* ]] + done + + # The bare fallback install must not run after a failed dep add. + run grep -x 'npm install' "${DDEV_STUB_LOG}" + [ "$status" -ne 0 ] + run grep -x 'npm install --package-lock' "${DDEV_STUB_LOG}" + [ "$status" -ne 0 ] +} + +@test "failed dep top-up on existing toolchain reports FAILED, not already present" { + write_node_deps_stub + write_root_package_json_without_deps + + # An existing root toolchain makes the installer skip the Node phase and + # instead offer to top up newly-missing deps. A failed top-up must be + # reported as FAILED, not masked by the pre-existing node_modules + # reading as "already present". + mkdir -p "${APP_ROOT}/node_modules/.bin" + printf '#!/bin/sh\nexit 0\n' > "${APP_ROOT}/node_modules/.bin/eslint" + chmod +x "${APP_ROOT}/node_modules/.bin/eslint" + + run python3 - <<'PY' +import os +import pty +import select +import signal +import sys + +addon_root = os.environ["ADDON_ROOT"] +app_root = os.environ["APP_ROOT"] +stub_bin = os.environ["STUB_BIN"] +env = os.environ.copy() +env["PATH"] = f"{stub_bin}:{env['PATH']}" +env["DDEV_APPROOT"] = app_root +env["DDEV_STUB_NPM_SAVE_DEV_EXIT"] = "1" +env["DCQ_INSTALL_DEPS"] = "skip" +env["DCQ_INSTALL_IDE_SETTINGS"] = "skip" +env["DCQ_INSTALL_GITIGNORE"] = "skip" +env["DCQ_PHPSTAN_LEVEL"] = "0" +for key in ("DCQ_INSTALL_NODE_DEPS", "DCQ_NONINTERACTIVE", "DDEV_NONINTERACTIVE"): + env.pop(key, None) + +cmd = ["bash", os.path.join(addon_root, "dcq-install.sh")] +prompts = { + b"Accept recommended settings for this install? [Y/n]": b"n\n", + b"Add missing dependencies with 'npm install --save-dev' in the project root?": b"y\n", +} + +seen = set() +buf = b"" +idle_ticks = 0 +timed_out = False + +pid, fd = pty.fork() +if pid == 0: + os.chdir(addon_root) + os.execvpe(cmd[0], cmd, env) + +try: + while True: + r, _, _ = select.select([fd], [], [], 2) + if fd in r: + data = os.read(fd, 1024) + if not data: + break + idle_ticks = 0 + sys.stdout.buffer.write(data) + sys.stdout.buffer.flush() + buf = (buf + data)[-16384:] + for prompt, response in prompts.items(): + if prompt in buf and prompt not in seen: + os.write(fd, response) + seen.add(prompt) + else: + idle_ticks += 1 + if idle_ticks > 30: + timed_out = True + break +except OSError: + pass + +if timed_out: + try: + os.kill(pid, signal.SIGTERM) + except OSError: + pass + +_, status = os.waitpid(pid, 0) +code = os.waitstatus_to_exitcode(status) +sys.exit(code) +PY + [ "$status" -eq 0 ] + [[ "$output" == *"Add missing dependencies with 'npm install --save-dev' in the project root?"* ]] + [[ "$output" == *"npm install --save-dev failed. You can retry manually."* ]] + [[ "$output" == *"Node toolchain: FAILED"* ]] + [[ "$output" != *"Node toolchain: already present (skipped)"* ]] +} + @test "non-interactive unset vars apply recommended defaults across installer phases" { unset DCQ_INSTALL_MODE unset DCQ_INSTALL_DEPS diff --git a/tests/test.bats b/tests/test.bats index a030956..cde43d5 100644 --- a/tests/test.bats +++ b/tests/test.bats @@ -2007,6 +2007,89 @@ JSON assert_container_file_exist "/var/www/html/node_modules/stylelint-prettier/package.json" } +# bats test_tags=node +@test "node install adds missing deps using core version constraints" { + set -u -o pipefail + export DCQ_INSTALL_DEPS=skip + export DCQ_INSTALL_NODE_DEPS=root + mkdir -p web/core + write_stub_package_json "web/core/package.json" + + # Root package.json declares no devDependencies, so every curated package + # is missing and the versioned `npm install --save-dev name@range` path + # runs. The semver ranges (^) must reach npm unescaped for this to work. + cat > package.json <<'JSON' +{ + "name": "dcq-test", + "private": true, + "engines": { + "node": ">= 20.0" + } +} +JSON + + run ddev add-on get "${DIR}" + assert_success + assert_output --partial "Node dependencies added (project root)." + assert_output --partial "Node toolchain installed (project root)." + run grep -F '"eslint": "^8.57.1"' package.json + assert_success + assert_container_file_exist "/var/www/html/node_modules/eslint-plugin-yml/package.json" + assert_container_file_exist "/var/www/html/node_modules/stylelint-prettier/package.json" +} + +# bats test_tags=node +@test "failed node dep add reports FAILED instead of success" { + set -u -o pipefail + export DCQ_INSTALL_DEPS=skip + export DCQ_INSTALL_NODE_DEPS=root + mkdir -p web/core + + # Core pins eslint to a version that cannot resolve; the root package.json + # has everything else, so the versioned dep add installs only eslint and + # fails. The installer must finish but report the failure prominently + # instead of falling back to a bare `npm install` and claiming success. + cat > web/core/package.json <<'JSON' +{ + "name": "drupal/core", + "devDependencies": { + "eslint": "99999.0.0" + } +} +JSON + cat > package.json <<'JSON' +{ + "name": "dcq-test", + "private": true, + "engines": { + "node": ">= 20.0" + }, + "devDependencies": { + "cspell": "^9.2.2", + "eslint-config-airbnb-base": "^15.0.0", + "eslint-config-prettier": "^10.1.8", + "eslint-plugin-import": "^2.32.0", + "eslint-plugin-jsdoc": "^61.2.1", + "eslint-plugin-no-jquery": "^3.1.1", + "eslint-plugin-prettier": "^5.5.4", + "eslint-plugin-yml": "^1.19.0", + "prettier": "^3.6.2", + "stylelint": "^16.25.0", + "stylelint-config-standard": "^38.0.0", + "stylelint-order": "^7.0.0", + "stylelint-prettier": "^5.0.3" + } +} +JSON + + run bash -lc "ddev add-on get \"${DIR}\" 2>&1" + assert_success + assert_output --partial "npm install --save-dev failed. You can retry manually." + assert_output --partial "Node toolchain install FAILED (project root)." + assert_output --partial "Node toolchain: FAILED" + refute_output --partial "Node toolchain installed (project root)." +} + # bats test_tags=node @test "node install failure does not crash the installer" { set -u -o pipefail @@ -2048,6 +2131,9 @@ JSON assert_success assert_output --partial "JS dependency install failed." assert_output --partial "You can retry manually" + assert_output --partial "Node toolchain install FAILED (project root)." + assert_output --partial "Node toolchain: FAILED" + refute_output --partial "Node toolchain installed (project root)." } # bats test_tags=full