Skip to content

Conversation

joshua-salsadigital
Copy link
Collaborator

@joshua-salsadigital joshua-salsadigital commented Oct 10, 2025

https://www.drupal.org/project/civictheme/issues/3550601

Checklist before requesting a review

  • I have formatted the subject to include ticket number as Issue #123456 by drupal_org_username: Issue title
  • I have added a link to the issue tracker
  • I have provided information in Changed section about WHY something was done if this was not a normal implementation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have run new and existing relevant tests locally with my changes, and they passed
  • I have provided screenshots, where applicable

Changed

  1. Updated file_validate_is_image to be replaced with Constraint plugins.

Screenshots

Summary by CodeRabbit

  • New Features
    • Expanded image upload support for site logo and footer background, including SVG and other supported formats.
  • Bug Fixes
    • Consistent validation across logo and footer image uploads to prevent unsupported or non-image files.
    • Improved error handling for invalid file types during uploads.

@joshua-salsadigital joshua-salsadigital self-assigned this Oct 10, 2025
@joshua-salsadigital joshua-salsadigital added the State: Needs review Pull requests needs a review from assigned developers label Oct 10, 2025
@joshua-salsadigital joshua-salsadigital changed the title [3550601] Drupal.org: The "file_validate_is_image" plugin does not exist Issue #3550601 by richardgaunt, fionamorrison23, joshua1234511: The "file_validate_is_image" plugin does not exist Oct 10, 2025
Copy link

coderabbitai bot commented Oct 10, 2025

Walkthrough

Introduces a centralized, dynamically computed list of allowed image extensions and applies it via shared validators (FileExtension and FileIsImage) to Logo and Footer background upload fields within the CivicTheme settings form.

Changes

Cohort / File(s) Summary
Settings form upload validators
web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php
Compute allowed image extensions from imageFactory, append svg, and create shared $upload_validators. Replace inline validators for Logo and Footer background uploads with the shared validators using FileExtension and FileIsImage.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Admin
  participant Form as Settings Form
  participant Validators as Upload Validators
  participant ImageFactory as Image Factory

  Admin->>Form: Open CivicTheme settings
  Form->>ImageFactory: Get supported image extensions
  ImageFactory-->>Form: Extensions list
  Form->>Validators: Configure FileExtension + FileIsImage
  Admin->>Form: Upload Logo/Footer image
  Form->>Validators: Validate file
  alt Valid
    Validators-->>Form: Pass
    Form-->>Admin: Save settings
  else Invalid
    Validators-->>Form: Fail
    Form-->>Admin: Show error
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • richardgaunt

Poem

I nudge my nose at uploads’ gates,
New rules hop in, to check our fates—
PNG, JPG, and SVG too,
Carrot-orange validators chew.
With tidy lists and image might,
Our logos gleam, our footers bright.
Thump-thump—all files validate right!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The PR title includes issue number and contributor names, making it overly verbose and focusing on the problem instead of summarizing the implemented solution. It does not provide a concise description of the primary code change, which is replacing the non-existent file_validate_is_image plugin with proper Constraint plugins. A clear title should highlight the actual modification rather than metadata or the bug description. Rename the PR title to a concise statement of the change, for example: “Replace file_validate_is_image plugin with Constraint-based validation for file uploads.”
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/3550601

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f6ad4e7 and 577ed23.

📒 Files selected for processing (1)
  • web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (1)
web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionBase.php (3)
  • form (77-77)
  • toFriendlyFilePath (100-106)
  • getDefaultFileScheme (114-116)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-and-push
  • GitHub Check: build-and-push
  • GitHub Check: commit
🔇 Additional comments (1)
web/themes/contrib/civictheme/src/Settings/CivicthemeSettingsFormSectionComponents.php (1)

88-90: Plugin IDs are correct
The plugin IDs 'FileExtension' and 'FileIsImage' match Drupal's File Validation constraint plugin definitions.

Copy link
Collaborator

@richardgaunt richardgaunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshua-salsadigital Thanks Josh tested, uploads are working again and the correct file extensions are being validated.

@richardgaunt richardgaunt merged commit 034e1a7 into develop Oct 14, 2025
17 of 19 checks passed
@richardgaunt richardgaunt deleted the feature/3550601 branch October 14, 2025 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

State: Needs review Pull requests needs a review from assigned developers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants