♻️ refactor: update command building to use array for PHPStan service

Morgy93 · Morgy93 · commit 32a8962d98d0 · 2026-01-12T14:01:30.000+01:00
diff --git a/src/services/phpstan-service.ts b/src/services/phpstan-service.ts
@@ -55,32 +55,35 @@ export class PhpstanService extends BasePhpToolService {
     /**
      * Build PHPStan command with configuration
      */
-    protected buildToolCommand(filePath: string): string {
-        let command = 'phpstan analyze --error-format=json --no-progress';
+    protected buildToolCommand(filePath: string): string[] {
+        const command = ['phpstan', 'analyze', '--error-format=json', '--no-progress'];
 
         // Use config file if specified, otherwise use individual settings
         if (this.config.configPath) {
             // Use specified config file
-            command += ` -c "${this.config.configPath}"`;
+            command.push('-c', this.config.configPath);
         } else {
             // Try to auto-detect common PHPStan config files
             const autoConfigPath = this.detectPhpstanConfig();
             if (autoConfigPath) {
                 console.log(`PHPStan: Auto-detected config file: ${autoConfigPath}`);
-                command += ` -c "${autoConfigPath}"`;
+                command.push('-c', autoConfigPath);
             } else {
                 // Use individual settings when no config file is found
-                command += ` --level=${this.config.level}`;
+                command.push(`--level=${this.config.level}`);
 
                 // Add exclude paths
                 if (this.config.excludePaths && this.config.excludePaths.length > 0) {
                     for (const excludePath of this.config.excludePaths) {
-                        command += ` --exclude="${excludePath}"`;
+                        command.push(`--exclude=${excludePath}`);
                     }
                 }
             }
         }
 
+        // Add the file to analyze
+        command.push(filePath);
+
         return command;
     }
 
diff --git a/src/shared/services/base-php-tool-service.ts b/src/shared/services/base-php-tool-service.ts
@@ -65,7 +65,7 @@ export abstract class BasePhpToolService {
     /**
      * Build the command to execute the tool
      */
-    protected abstract buildToolCommand(relativePath: string): string;
+    protected abstract buildToolCommand(relativePath: string): string[];
 
     /**
      * Process the tool output and convert to diagnostics
@@ -193,7 +193,7 @@ export abstract class BasePhpToolService {
         try {
             // PHPStan returns exit code 1 when errors are found, which is normal
             const allowedExitCodes = this.toolName === 'phpstan' ? [0, 1] : [0];
-            console.log(`Executing ${this.displayName} command: ${toolCommand}`);
+            console.log(`Executing ${this.displayName} command: ${toolCommand.join(' ')}`);
 
             const output = DdevUtils.execDdev(toolCommand, workspaceFolder.uri.fsPath, allowedExitCodes);
             console.log(`${this.displayName} output length: ${output.length} characters`);
diff --git a/src/shared/utils/ddev-utils.ts b/src/shared/utils/ddev-utils.ts
@@ -19,7 +19,9 @@
  */
 
 import * as vscode from 'vscode';
-import { execSync } from 'child_process';
+import { spawnSync } from 'child_process';
+import * as fs from 'fs';
+import * as path from 'path';
 
 /**
  * DDEV project validation result
@@ -46,11 +48,8 @@ export class DdevUtils {
      */
     public static hasDdevProject(workspacePath: string): boolean {
         try {
-            const ddevConfig = execSync('test -f .ddev/config.yaml && echo "exists"', {
-                cwd: workspacePath,
-                encoding: 'utf-8'
-            });
-            return ddevConfig.includes('exists');
+            const configPath = path.join(workspacePath, '.ddev', 'config.yaml');
+            return fs.existsSync(configPath);
         } catch (error) {
             return false;
         }
@@ -64,11 +63,11 @@ export class DdevUtils {
      */
     public static isDdevRunning(workspacePath: string): boolean {
         try {
-            execSync('ddev exec echo "test"', {
+            const result = spawnSync('ddev', ['exec', 'echo', 'test'], {
                 cwd: workspacePath,
-                stdio: 'ignore'
+                encoding: 'utf-8'
             });
-            return true;
+            return result.status === 0;
         } catch (error) {
             return false;
         }
@@ -83,7 +82,7 @@ export class DdevUtils {
      */
     public static isToolInstalled(toolName: string, workspacePath: string): boolean {
         try {
-            this.execDdev(`${toolName} --version`, workspacePath);
+            this.execDdev([toolName, '--version'], workspacePath);
             return true;
         } catch (error) {
             return false;
@@ -109,7 +108,7 @@ export class DdevUtils {
 
         // Try to run the tool
         try {
-            this.execDdev(`${toolName} --version`, workspacePath);
+            this.execDdev([toolName, '--version'], workspacePath);
 
             return {
                 isValid: true
@@ -174,37 +173,62 @@ export class DdevUtils {
     /**
      * Execute a command in the DDEV container
      *
-     * @param command Command to execute
+     * @param command Command to execute (as array of strings)
      * @param workspacePath Path to the workspace
      * @param allowedExitCodes Array of exit codes that should not throw (default: [0])
      * @returns Output of the command
      * @throws Error if the command fails with disallowed exit code
      */
-    public static execDdev(command: string, workspacePath: string, allowedExitCodes: number[] = [0]): string {
+    public static execDdev(command: string[], workspacePath: string, allowedExitCodes: number[] = [0]): string {
         try {
-            // Wrap command in bash -c to allow setting environment variables (specifically disabling Xdebug)
-            // This fixes issues where Xdebug causes the command to hang or run slowly
-            // We use single quotes for the bash command and escape any single quotes in the original command
-            const escapedCommand = command.replace(/'/g, "'\\''");
-            return execSync(`ddev exec bash -c 'XDEBUG_MODE=off ${escapedCommand}'`, {
+            // Use spawnSync to avoid shell injection and safely pass arguments
+            // We use 'env' to set environment variables inside the container
+            const args = ['exec', 'env', 'XDEBUG_MODE=off', ...command];
+
+            const result = spawnSync('ddev', args, {
                 cwd: workspacePath,
                 encoding: 'utf-8'
             });
-        } catch (error: any) {
-            // Check if this is an acceptable exit code (e.g., PHPStan returns 1 when errors are found)
-            if (error.status && allowedExitCodes.includes(error.status)) {
+
+            if (result.error) {
+                throw result.error;
+            }
+
+            // Check if this is an acceptable exit code
+            if (result.status !== null && allowedExitCodes.includes(result.status)) {
                 // Return stdout even if exit code is non-zero but allowed
-                console.log(`Command exited with allowed code ${error.status}: ${command}`);
-                return error.stdout || '';
+                if (result.status !== 0) {
+                    console.log(`Command exited with allowed code ${result.status}: ${command.join(' ')}`);
+                }
+                return result.stdout || '';
+            }
+
+            if (result.status !== 0) {
+                 // Enhance error message with more details
+                const enhancedError = new Error(result.stderr || 'Command execution failed');
+                enhancedError.name = 'CommandError';
+                (enhancedError as any).status = result.status;
+                (enhancedError as any).stderr = result.stderr;
+                (enhancedError as any).stdout = result.stdout;
+                (enhancedError as any).command = `ddev exec ${command.join(' ')}`;
+                (enhancedError as any).workspacePath = workspacePath;
+                throw enhancedError;
+            }
+
+            return result.stdout;
+        } catch (error: any) {
+             // If error was already thrown above, rethrow it
+            if (error.name === 'CommandError') {
+                throw error;
             }
 
-            // Enhance error message with more details
+            // Handle unexpected errors
             const enhancedError = new Error(error.message || 'Command execution failed');
             enhancedError.name = error.name || 'CommandError';
             (enhancedError as any).status = error.status;
             (enhancedError as any).stderr = error.stderr;
             (enhancedError as any).stdout = error.stdout;
-            (enhancedError as any).command = `ddev exec ${command}`;
+            (enhancedError as any).command = `ddev exec ${command.join(' ')}`;
             (enhancedError as any).workspacePath = workspacePath;
 
             throw enhancedError;
diff --git a/src/test/ddev-utils.test.ts b/src/test/ddev-utils.test.ts
@@ -21,73 +21,85 @@ import * as assert from 'assert';
 import * as sinon from 'sinon';
 import { afterEach, beforeEach } from 'mocha';
 import { DdevUtils } from '../shared/utils/ddev-utils';
+import * as cp from 'child_process';
+import * as fs from 'fs';
 
 suite('DdevUtils Test Suite', () => {
     let sandbox: sinon.SinonSandbox;
-    let execSyncStub: sinon.SinonStub;
+    let spawnSyncStub: sinon.SinonStub;
+    let existsSyncStub: sinon.SinonStub;
 
     beforeEach(() => {
         sandbox = sinon.createSandbox();
-        // Mock the entire child_process module
-        const childProcess = require('child_process');
-        execSyncStub = sandbox.stub(childProcess, 'execSync');
+        // Use require to get the module to ensure we can stub it
+        const cp = require('child_process');
+        const fs = require('fs');
+
+        // Try to stub, but handle if it fails (basic check)
+        try {
+            spawnSyncStub = sandbox.stub(cp, 'spawnSync');
+            existsSyncStub = sandbox.stub(fs, 'existsSync');
+        } catch (e) {
+            console.error('Failed to stub modules:', e);
+            throw e;
+        }
     });
 
     afterEach(() => {
         sandbox.restore();
     });
 
     test('hasDdevProject returns true when .ddev/config.yaml exists', () => {
-        execSyncStub.returns('exists\n');
+        existsSyncStub.returns(true);
 
         const result = DdevUtils.hasDdevProject('/test/workspace');
 
         assert.strictEqual(result, true);
-        assert.strictEqual(execSyncStub.calledOnce, true);
+        assert.strictEqual(existsSyncStub.calledOnce, true);
     });
 
     test('hasDdevProject returns false when .ddev/config.yaml does not exist', () => {
-        execSyncStub.throws(new Error('File not found'));
+        existsSyncStub.returns(false);
 
         const result = DdevUtils.hasDdevProject('/test/workspace');
 
         assert.strictEqual(result, false);
     });
 
     test('isDdevRunning returns true when DDEV container is running', () => {
-        execSyncStub.returns('');
+        spawnSyncStub.returns({ status: 0, stdout: 'test\n' });
 
         const result = DdevUtils.isDdevRunning('/test/workspace');
 
         assert.strictEqual(result, true);
     });
 
     test('isDdevRunning returns false when DDEV container is not running', () => {
-        execSyncStub.throws(new Error('Container not running'));
+        spawnSyncStub.returns({ status: 1, stderr: 'Container not running' });
 
         const result = DdevUtils.isDdevRunning('/test/workspace');
 
         assert.strictEqual(result, false);
     });
 
     test('isToolInstalled returns true when tool is available', () => {
-        execSyncStub.returns('PHPStan 1.10.0\n');
+        spawnSyncStub.returns({ status: 0, stdout: 'PHPStan 1.10.0\n' });
 
         const result = DdevUtils.isToolInstalled('phpstan', '/test/workspace');
 
         assert.strictEqual(result, true);
     });
 
     test('isToolInstalled returns false when tool is not available', () => {
-        execSyncStub.throws(new Error('Command not found'));
+        spawnSyncStub.returns({ status: 1, stderr: 'Command not found' });
 
         const result = DdevUtils.isToolInstalled('phpstan', '/test/workspace');
 
         assert.strictEqual(result, false);
     });
 
     test('validateDdevTool returns invalid result when no DDEV project found', () => {
-        execSyncStub.throws(new Error('File not found'));
+        existsSyncStub.returns(false);
 
         const result = DdevUtils.validateDdevTool('phpstan', '/test/workspace');
 
@@ -97,10 +109,10 @@ suite('DdevUtils Test Suite', () => {
     });
 
     test('validateDdevTool returns valid result when tool is available', () => {
-        // First call (hasDdevProject) succeeds
-        execSyncStub.onFirstCall().returns('exists\n');
-        // Second call (tool version check) succeeds
-        execSyncStub.onSecondCall().returns('PHPStan 1.10.0\n');
+        // hasDdevProject
+        existsSyncStub.returns(true);
+        // execDdev (tool check)
+        spawnSyncStub.returns({ status: 0, stdout: 'PHPStan 1.10.0\n' });
 
         const result = DdevUtils.validateDdevTool('phpstan', '/test/workspace');
 
@@ -109,12 +121,12 @@ suite('DdevUtils Test Suite', () => {
     });
 
     test('validateDdevTool returns error message for DDEV issues', () => {
-        // First call (hasDdevProject) succeeds
-        execSyncStub.onFirstCall().returns('exists\n');
-        // Second call (tool version check) fails
-        execSyncStub.onSecondCall().throws(new Error('Container not running'));
-        // Third call (isDdevRunning) fails
-        execSyncStub.onThirdCall().throws(new Error('DDEV not running'));
+        // hasDdevProject
+        existsSyncStub.returns(true);
+        // execDdev (tool check) fails
+        spawnSyncStub.onFirstCall().returns({ status: 1, stderr: 'Container not running' });
+        // isDdevRunning fails
+        spawnSyncStub.onSecondCall().returns({ status: 1, stderr: 'DDEV not running' });
 
         const result = DdevUtils.validateDdevTool('phpstan', '/test/workspace');
 
@@ -124,12 +136,12 @@ suite('DdevUtils Test Suite', () => {
     });
 
     test('validateDdevTool returns tool not found message when tool is missing', () => {
-        // First call (hasDdevProject) succeeds
-        execSyncStub.onFirstCall().returns('exists\n');
-        // Second call (tool version check) fails
-        execSyncStub.onSecondCall().throws(new Error('Command not found'));
-        // Third call (isDdevRunning) succeeds
-        execSyncStub.onThirdCall().returns('');
+        // hasDdevProject
+        existsSyncStub.returns(true);
+        // execDdev (tool check) fails
+        spawnSyncStub.onFirstCall().returns({ status: 127, stderr: 'Command not found' });
+        // isDdevRunning succeeds
+        spawnSyncStub.onSecondCall().returns({ status: 0, stdout: 'test\n' });
 
         const result = DdevUtils.validateDdevTool('phpstan', '/test/workspace');
 
@@ -139,27 +151,35 @@ suite('DdevUtils Test Suite', () => {
         assert.ok(result.userMessage?.includes('phpstan/phpstan'));
     });
 
-    test('execDdev wraps command with XDEBUG_MODE=off', () => {
-        execSyncStub.returns('output');
+    test('execDdev passes args array correctly', () => {
+        spawnSyncStub.returns({ status: 0, stdout: 'output' });
 
-        const result = DdevUtils.execDdev('phpstan analyze', '/test/workspace');
+        const result = DdevUtils.execDdev(['phpstan', 'analyze'], '/test/workspace');
 
         assert.strictEqual(result, 'output');
-        assert.strictEqual(execSyncStub.calledOnce, true);
-        const callArgs = execSyncStub.firstCall.args;
-        assert.ok(callArgs[0].includes("XDEBUG_MODE=off"));
-        assert.ok(callArgs[0].includes("bash -c"));
-        assert.ok(callArgs[0].includes("'XDEBUG_MODE=off phpstan analyze'"));
+        assert.strictEqual(spawnSyncStub.calledOnce, true);
+        const callArgs = spawnSyncStub.firstCall.args;
+        assert.strictEqual(callArgs[0], 'ddev');
+        assert.deepStrictEqual(callArgs[1], ['exec', 'env', 'XDEBUG_MODE=off', 'phpstan', 'analyze']);
+        assert.strictEqual(callArgs[2].cwd, '/test/workspace');
     });
 
-    test('execDdev escapes single quotes in command', () => {
-        execSyncStub.returns('output');
+    test('execDdev throws on non-zero exit code', () => {
+        spawnSyncStub.returns({ status: 1, stderr: 'error', stdout: '' });
 
-        const result = DdevUtils.execDdev("echo 'hello'", '/test/workspace');
+        assert.throws(() => {
+            DdevUtils.execDdev(['ls'], '/test/workspace');
+        }, (err: any) => {
+            return err.status === 1 && err.stderr === 'error';
+        });
+    });
 
-        assert.strictEqual(result, 'output');
-        const callArgs = execSyncStub.firstCall.args;
-        // Should be: ddev exec bash -c 'XDEBUG_MODE=off echo '\''hello'\'''
-        assert.ok(callArgs[0].includes("'XDEBUG_MODE=off echo '\\''hello'\\'''"));
+    test('execDdev returns stdout on allowed non-zero exit code', () => {
+        spawnSyncStub.returns({ status: 1, stdout: 'partial output' });
+
+        const result = DdevUtils.execDdev(['ls'], '/test/workspace', [0, 1]);
+
+        assert.strictEqual(result, 'partial output');
     });
 });
+
