Skip to content

feat: add SonarQube workflow template for PHP/Drupal#7

Merged
MichaelFisher1997 merged 1 commit intomainfrom
feature/sonarqube-workflow
Feb 12, 2026
Merged

feat: add SonarQube workflow template for PHP/Drupal#7
MichaelFisher1997 merged 1 commit intomainfrom
feature/sonarqube-workflow

Conversation

@MichaelFisher1997
Copy link
Collaborator

Summary

Adds SonarQube workflow template for PHP/Drupal projects.

Changes

  • sonarqube.yml - Self-hosted SonarQube scan workflow with:

    • Push/PR triggers on main/master
    • PHP 8.3 setup with Composer caching
    • SonarQube scan action
    • SARIF export to GitHub Security Tab
    • Secrets: SONAR_TOKEN, SONAR_HOST_URL
  • sonar-project.properties - PHP/Drupal configuration template:

    • Project identification placeholders
    • Drupal source paths (web/modules/custom, web/themes/custom)
    • Comprehensive exclusions (vendor, core, contrib, etc.)
    • GitHub PR integration
  • Registry updates:

    • Discovers .properties files alongside .yml/.yaml
    • Metadata id field now overrides derived workflow ID

Testing

  • All 30 existing tests pass
  • Workflow discovered correctly by registry

- Add sonarqube.yml workflow with self-hosted SonarQube scan
- Add sonar-project.properties configuration template
- Includes SARIF export to GitHub Security Tab
- Update registry to discover .properties files
- Allow metadata.id to override derived workflow ID
@opencode-agent
Copy link

📋 Summary

No linked issues found in PR description.

This PR adds a comprehensive SonarQube workflow template for PHP/Drupal projects, including a GitHub Actions workflow with SARIF export and a properties configuration file. It also extends the registry to support .properties files and allows metadata id fields to override derived workflow IDs. The implementation is well-structured with proper metadata blocks and follows the existing patterns in the codebase.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified

⚠️ High Priority Issues (Should Fix)

[HIGH] src/core/registry.ts:307,310 - .properties files not handled in groupByBaseName
Confidence: High
Description: The groupByBaseName method uses regex /\.ya?ml$/ to strip extensions, but .properties files retain their extension. This causes incorrect base name grouping for properties files.
Impact: Properties files will be grouped incorrectly (e.g., sonar-project.properties won't match sonar-project as expected), potentially creating duplicate or orphaned workflow entries.
Suggested Fix: Update the regex in lines 307 and 310 to also handle .properties extension:

// Line 307
const names = new Set(files.map((file) => file.replace(/\.(ya?ml|properties)$/, '')));

// Line 310  
const name = file.replace(/\.(ya?ml|properties)$/, '');

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] src/core/registry.ts:222 - ID override precedence could cause collisions
Confidence: Medium
Description: Allowing metadata id to override the derived ID provides flexibility but could lead to naming collisions if multiple workflows define the same custom ID.
Impact: If two workflows specify the same id in metadata, the second one will silently overwrite the first in the registry Map.
Suggested Fix: Add collision detection before setting the workflow:

const workflowId = metadata.id ?? `${args.category.id}/${args.workflowType}`;
if (workflows.has(workflowId)) {
  console.warn(`Warning: Duplicate workflow ID '${workflowId}' detected. Skipping...`);
  return null;
}

[MEDIUM] workflows/ci/sonarqube/sonarqube.yml:99 - SARIF upload only on push to main/master
Confidence: Low
Description: The SARIF upload step has a conditional that only runs on push events to main/master branches, which means PRs won't have security findings uploaded to the Security tab.
Impact: Developers won't see SonarQube security findings in the GitHub Security tab when reviewing PRs - only after merge.
Suggested Fix: Consider whether this is intentional. If you want PR findings in the Security tab, remove or modify the condition:

# Option 1: Upload on all events
if: always()

# Option 2: Upload on PRs too  
if: github.event_name == 'push' || github.event_name == 'pull_request'

ℹ️ Low Priority Suggestions (Optional)

[LOW] workflows/ci/sonarqube/sonar-project.properties:46-49 - Sonar exclusions use += syntax
Confidence: Medium
Description: The sonar-project.properties uses += operator to append to exclusions. While valid, some users may not be familiar with this syntax.
Impact: Users copying this config might be confused about whether to use = or +=.
Suggested Fix: Consider consolidating into single line for clarity, or add a comment explaining the syntax:

# Exclusions combined with commas (single line is clearer)
sonar.exclusions=**/vendor/**,**/node_modules/**,**/libraries/**,**/dist/**,**/css/**,**/*.min.js,**/*.min.css,**/tests/**,**/test/**,**/spec/**,web/core/**,web/modules/contrib/**,web/themes/contrib/**,web/profiles/contrib/**,web/libraries/**,**/config/**,**/files/**

[LOW] workflows/ci/sonarqube/sonarqube.yml:59 - Hardcoded PHP version
Confidence: Low
Description: PHP 8.3 is hardcoded in the workflow.
Impact: Teams using different PHP versions will need to modify the workflow.
Suggested Fix: Consider if this should be configurable via workflow input or better documented in the description.

📊 SOLID Principles Score

Principle Score Notes
Single Responsibility 9 Workflow file, config file, and registry changes each have clear, focused purposes
Open/Closed 8 Registry extended to support .properties without modifying existing logic extensively
Liskov Substitution 9 New files follow existing workflow patterns; no inheritance issues
Interface Segregation 9 Metadata blocks are focused and don't force unused fields
Dependency Inversion 8 Registry depends on abstractions; workflow depends on external actions via interfaces
Average 8.6

🎯 Final Assessment

Overall Confidence Score: 85%

Confidence Breakdown:

  • Code Quality: 90% (clean implementation, follows conventions)
  • Completeness: 85% (minor gap in .properties extension handling)
  • Risk Level: 80% (low risk but potential for ID collisions)
  • Test Coverage: 85% (existing tests pass, but edge case in grouping not covered)

Merge Readiness:

  • All critical issues resolved
  • SOLID average score >= 6.0 (8.6 achieved)
  • Overall confidence >= 60% (85% achieved)
  • No security concerns
  • Tests present and passing (30 existing tests pass)

Verdict:

MERGE WITH FIXES

The PR is well-implemented and follows established patterns. Fix the .properties extension handling in groupByBaseName (lines 307, 310) before merging to ensure proper file grouping. The ID collision warning is optional but recommended for robustness.

New%20session%20-%202026-02-12T23%3A55%3A57.621Z
opencode session  |  github run

@MichaelFisher1997 MichaelFisher1997 merged commit 58b9b7b into main Feb 12, 2026
2 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.

1 participant