[#3549887] Added additional component validation #1430
Conversation
WalkthroughAdds a new custom Drupal module "Single Directory Component Validator" providing a Drush command to validate SDC component YAML and Twig templates, a Twig validator hook, Drush service registration, PHPUnit config, and kernel tests covering valid and error scenarios. Changes
Sequence Diagram(s)sequenceDiagram
actor Dev as Developer
participant Drush as Drush CLI
participant Cmd as ValidateComponentCommand
participant FS as File System
participant YAML as Symfony Yaml
participant Twig as Twig Environment
participant CV as ComponentValidator
Dev->>Drush: run sdc:validate --components_path
Drush->>Cmd: validateComponentDefinitions(path)
Cmd->>FS: confirm directory exists
alt missing
Cmd-->>Drush: throw/return error (missing dir)
else exists
Cmd->>FS: discover *.component.yml files
alt none found
Cmd-->>Drush: throw/return error (no files)
else found
loop per component file
Cmd->>YAML: parse file
alt YAML error
Cmd-->>Drush: record YAML failure
else parsed
Cmd->>FS: locate Twig template
Cmd->>Twig: parse template AST
alt Twig parse error
Cmd-->>Drush: record template failure
else parsed
Cmd->>CV: validate merged component metadata
Cmd->>Cmd: validateSlots(component, node)
alt validation errors
Cmd-->>Drush: record validation failure(s)
else valid
Cmd-->>Drush: record success
end
end
end
end
Cmd-->>Drush: output summary (counts, failures)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php (1)
106-138: Add template file for completeness.While this test may pass because the YAML parse error occurs before template validation, add a template file for consistency and to match the expected component structure.
Add after line 125:
+ // Create template file. + file_put_contents( + $this->testComponentsDir . '/invalid-yaml.twig', + '{% block content %}{% endblock %}' + ); + $this->expectException(\Exception::class);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web/modules/custom/sdc_validator/drush.services.yml(1 hunks)web/modules/custom/sdc_validator/phpunit.xml.dist(1 hunks)web/modules/custom/sdc_validator/sdc_validator.info.yml(1 hunks)web/modules/custom/sdc_validator/sdc_validator.module(1 hunks)web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php(1 hunks)web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php (1)
web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php (3)
ValidateComponentCommand(23-209)validateComponentDefinitions(55-141)__construct(38-43)
web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php (1)
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php (2)
__construct(260-263)output(268-270)
⏰ 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 (4)
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php (4)
46-56: LGTM!The setUp method correctly initializes test dependencies and creates an isolated temporary directory for test components.
61-67: LGTM!The tearDown method properly cleans up test artifacts and calls the parent method.
173-190: LGTM!Both tests correctly validate early error conditions (non-existent directory and empty directory) that are caught before component processing begins.
250-279: LGTM!The mock class cleanly intercepts output for testing by overriding
output()and providing a buffer accessor. The implementation correctly extends the command class and maintains the parent's contract.
| $component_path_parts = explode('/', $components_path); | ||
| array_filter($component_path_parts); | ||
| $component_base_identifier = $component_path_parts[count($component_path_parts) - 2] ?? NULL; | ||
| if ($component_base_identifier === NULL) { | ||
| throw new \Exception('❌ Cannot validate components that are not located in a theme or a module: ' . $components_path); | ||
| } |
There was a problem hiding this comment.
Fix path sanitisation before deriving the component ID.
array_filter($component_path_parts); does nothing without reassigning the filtered result. With a trailing slash (common when tab-completing paths) the last segment becomes an empty string, so $component_base_identifier resolves to 'components', and find() looks up components:<name> instead of the actual theme/module. The command then fails despite a valid directory. Reassign the filtered segments (and reindex) before using them:
- $component_path_parts = explode('/', $components_path);
- array_filter($component_path_parts);
+ $component_path_parts = array_values(array_filter(
+ explode('/', $components_path),
+ static fn ($segment) => $segment !== ''
+ ));This guarantees the second-last element really is the extension machine name.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $component_path_parts = explode('/', $components_path); | |
| array_filter($component_path_parts); | |
| $component_base_identifier = $component_path_parts[count($component_path_parts) - 2] ?? NULL; | |
| if ($component_base_identifier === NULL) { | |
| throw new \Exception('❌ Cannot validate components that are not located in a theme or a module: ' . $components_path); | |
| } | |
| $component_path_parts = array_values(array_filter( | |
| explode('/', $components_path), | |
| static fn ($segment) => $segment !== '' | |
| )); | |
| $component_base_identifier = $component_path_parts[count($component_path_parts) - 2] ?? NULL; | |
| if ($component_base_identifier === NULL) { | |
| throw new \Exception('❌ Cannot validate components that are not located in a theme or a module: ' . $components_path); | |
| } |
🤖 Prompt for AI Agents
In web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php
around lines 60 to 65, the code calls array_filter($component_path_parts)
without reassigning, so trailing slashes leave empty segments and the
second-last element can be wrong; reassign the filtered and reindexed array
(e.g. $component_path_parts = array_values(array_filter($component_path_parts)))
before computing $component_base_identifier, then compute the second-last
element via count($component_path_parts) - 2 and keep the existing
NULL/exception guard when there aren’t enough segments.
web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php
Outdated
Show resolved
Hide resolved
| protected static $modules = [ | ||
| 'sdc_validator', | ||
| ]; |
There was a problem hiding this comment.
Add 'sdc' module to test dependencies.
The ValidateComponentCommand depends on ComponentPluginManager (service plugin.manager.sdc) which is provided by Drupal core's sdc module. Without enabling it, the test will fail when attempting to instantiate the service at line 55.
Apply this diff:
protected static $modules = [
+ 'sdc',
'sdc_validator',
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected static $modules = [ | |
| 'sdc_validator', | |
| ]; | |
| protected static $modules = [ | |
| 'sdc', | |
| 'sdc_validator', | |
| ]; |
🤖 Prompt for AI Agents
In
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php
around lines 24 to 26, the test's static $modules array is missing the 'sdc'
module which provides plugin.manager.sdc; update the $modules list to include
'sdc' alongside 'sdc_validator' so the ComponentPluginManager service is
available during the test bootstrap.
| public function testValidComponent(): void { | ||
| // Create a valid component file. | ||
| file_put_contents( | ||
| $this->testComponentsDir . '/valid.component.yml', | ||
| <<<YAML | ||
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | ||
| name: Valid Component | ||
| status: stable | ||
| description: A valid test component | ||
| props: | ||
| type: object | ||
| properties: | ||
| theme: | ||
| type: string | ||
| enum: | ||
| - light | ||
| - dark | ||
| slots: | ||
| content: | ||
| title: Content | ||
| description: Main content slot | ||
| YAML | ||
| ); | ||
|
|
||
| $this->command->validateComponentDefinitions($this->testComponentsDir); | ||
| $output = $this->command->getOutputBuffer(); | ||
|
|
||
| $this->assertStringContainsString('✅ 1 components are valid', $output); | ||
| $this->assertStringContainsString('✨ All components are valid', $output); | ||
| } |
There was a problem hiding this comment.
Add template file for the component.
The test creates only a .component.yml file but ValidateComponentCommand::validateComponentDefinitions() requires a template. At lines 88-93 of ValidateComponentCommand.php, it retrieves the template path and throws an exception if null. This test will fail without a corresponding .twig file.
Add after line 94:
+ // Create corresponding template file.
+ file_put_contents(
+ $this->testComponentsDir . '/valid.twig',
+ '{% block content %}{% endblock %}'
+ );
+
$this->command->validateComponentDefinitions($this->testComponentsDir);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testValidComponent(): void { | |
| // Create a valid component file. | |
| file_put_contents( | |
| $this->testComponentsDir . '/valid.component.yml', | |
| <<<YAML | |
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | |
| name: Valid Component | |
| status: stable | |
| description: A valid test component | |
| props: | |
| type: object | |
| properties: | |
| theme: | |
| type: string | |
| enum: | |
| - light | |
| - dark | |
| slots: | |
| content: | |
| title: Content | |
| description: Main content slot | |
| YAML | |
| ); | |
| $this->command->validateComponentDefinitions($this->testComponentsDir); | |
| $output = $this->command->getOutputBuffer(); | |
| $this->assertStringContainsString('✅ 1 components are valid', $output); | |
| $this->assertStringContainsString('✨ All components are valid', $output); | |
| } | |
| public function testValidComponent(): void { | |
| // Create a valid component file. | |
| file_put_contents( | |
| $this->testComponentsDir . '/valid.component.yml', | |
| <<<YAML | |
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | |
| name: Valid Component | |
| status: stable | |
| description: A valid test component | |
| props: | |
| type: object | |
| properties: | |
| theme: | |
| type: string | |
| enum: | |
| - light | |
| - dark | |
| slots: | |
| content: | |
| title: Content | |
| description: Main content slot | |
| YAML | |
| ); | |
| // Create corresponding template file. | |
| file_put_contents( | |
| $this->testComponentsDir . '/valid.twig', | |
| '{% block content %}{% endblock %}' | |
| ); | |
| $this->command->validateComponentDefinitions($this->testComponentsDir); | |
| $output = $this->command->getOutputBuffer(); | |
| $this->assertStringContainsString('✅ 1 components are valid', $output); | |
| $this->assertStringContainsString('✨ All components are valid', $output); | |
| } |
🤖 Prompt for AI Agents
In
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php
around lines 72 to 101, the test writes only valid.component.yml but
ValidateComponentCommand::validateComponentDefinitions() expects a matching
template file and will throw if none is found; create a corresponding template
file (valid.component.twig) in $this->testComponentsDir before calling
validateComponentDefinitions() with a minimal valid Twig template (e.g. a simple
div or placeholder) so the validator can find the template; ensure the file is
created in the same test setup and cleaned up with other test artifacts.
| public function testEmptySlotsProperty(): void { | ||
| // Create component with empty slots. | ||
| file_put_contents( | ||
| $this->testComponentsDir . '/empty-slots.component.yml', | ||
| <<<YAML | ||
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | ||
| name: Empty Slots Component | ||
| status: stable | ||
| props: | ||
| type: object | ||
| slots: | ||
| YAML | ||
| ); | ||
|
|
||
| $this->expectException(\Exception::class); | ||
| $this->expectExceptionMessage('Component validation failed.'); | ||
|
|
||
| try { | ||
| $this->command->validateComponentDefinitions($this->testComponentsDir); | ||
| } | ||
| catch (\Exception $exception) { | ||
| $output = $this->command->getOutputBuffer(); | ||
| $this->assertStringContainsString('[slots] NULL value found, but an object is required', $output); | ||
| throw $exception; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add template file for the component.
The test needs a corresponding .twig template to reach the slots validation logic being tested.
Add after line 155:
+ // Create template file.
+ file_put_contents(
+ $this->testComponentsDir . '/empty-slots.twig',
+ '{% block content %}{% endblock %}'
+ );
+
$this->expectException(\Exception::class);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testEmptySlotsProperty(): void { | |
| // Create component with empty slots. | |
| file_put_contents( | |
| $this->testComponentsDir . '/empty-slots.component.yml', | |
| <<<YAML | |
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | |
| name: Empty Slots Component | |
| status: stable | |
| props: | |
| type: object | |
| slots: | |
| YAML | |
| ); | |
| $this->expectException(\Exception::class); | |
| $this->expectExceptionMessage('Component validation failed.'); | |
| try { | |
| $this->command->validateComponentDefinitions($this->testComponentsDir); | |
| } | |
| catch (\Exception $exception) { | |
| $output = $this->command->getOutputBuffer(); | |
| $this->assertStringContainsString('[slots] NULL value found, but an object is required', $output); | |
| throw $exception; | |
| } | |
| } | |
| public function testEmptySlotsProperty(): void { | |
| // Create component with empty slots. | |
| file_put_contents( | |
| $this->testComponentsDir . '/empty-slots.component.yml', | |
| <<<YAML | |
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | |
| name: Empty Slots Component | |
| status: stable | |
| props: | |
| type: object | |
| slots: | |
| YAML | |
| ); | |
| // Create template file. | |
| file_put_contents( | |
| $this->testComponentsDir . '/empty-slots.twig', | |
| '{% block content %}{% endblock %}' | |
| ); | |
| $this->expectException(\Exception::class); | |
| $this->expectExceptionMessage('Component validation failed.'); | |
| try { | |
| $this->command->validateComponentDefinitions($this->testComponentsDir); | |
| } | |
| catch (\Exception $exception) { | |
| $output = $this->command->getOutputBuffer(); | |
| $this->assertStringContainsString('[slots] NULL value found, but an object is required', $output); | |
| throw $exception; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php
around lines 143 to 168, the test creates an empty-slots.component.yml but does
not add the required template file so the slots-validation path is never
exercised; create a corresponding template file named empty-slots.component.twig
inside the same test components directory ($this->testComponentsDir) immediately
after line 155, with a minimal valid Twig template (e.g., a simple wrapper that
references any slot placeholders or children) so the component can be loaded and
the slots validation logic runs.
| public function testMultipleComponentsWithFailures(): void { | ||
| // Create valid component. | ||
| file_put_contents( | ||
| $this->testComponentsDir . '/valid.component.yml', | ||
| <<<YAML | ||
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | ||
| name: Valid Component | ||
| status: stable | ||
| description: A valid component | ||
| props: | ||
| type: object | ||
| slots: | ||
| content: | ||
| title: Content | ||
| YAML | ||
| ); | ||
|
|
||
| // Create invalid component. | ||
| file_put_contents( | ||
| $this->testComponentsDir . '/invalid.component.yml', | ||
| <<<YAML | ||
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | ||
| name: Invalid Component | ||
| status: stable | ||
| props: | ||
| type: object | ||
| properties: | ||
| test | ||
| invalid: yaml | ||
| slots: | ||
| YAML | ||
| ); | ||
|
|
||
| $this->expectException(\Exception::class); | ||
| $this->expectExceptionMessage('Component validation failed.'); | ||
|
|
||
| try { | ||
| $this->command->validateComponentDefinitions($this->testComponentsDir); | ||
| } | ||
| catch (\Exception $exception) { | ||
| $output = $this->command->getOutputBuffer(); | ||
| // Should show 1 valid component. | ||
| $this->assertStringContainsString('✅ 1 components are valid', $output); | ||
| // Should show the failure. | ||
| $this->assertStringContainsString('Failed components:', $output); | ||
| $this->assertStringContainsString('Mapping values are not allowed', $output); | ||
| throw $exception; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add template files for both components.
Both the valid and invalid components need corresponding .twig templates to properly test the mixed success/failure scenario and summary output.
Add after line 210:
+ // Create valid template.
+ file_put_contents(
+ $this->testComponentsDir . '/valid.twig',
+ '{% block content %}{% endblock %}'
+ );
+
// Create invalid component.Add after line 226:
+ // Create invalid template.
+ file_put_contents(
+ $this->testComponentsDir . '/invalid.twig',
+ '{% block content %}{% endblock %}'
+ );
+
$this->expectException(\Exception::class);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testMultipleComponentsWithFailures(): void { | |
| // Create valid component. | |
| file_put_contents( | |
| $this->testComponentsDir . '/valid.component.yml', | |
| <<<YAML | |
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | |
| name: Valid Component | |
| status: stable | |
| description: A valid component | |
| props: | |
| type: object | |
| slots: | |
| content: | |
| title: Content | |
| YAML | |
| ); | |
| // Create invalid component. | |
| file_put_contents( | |
| $this->testComponentsDir . '/invalid.component.yml', | |
| <<<YAML | |
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | |
| name: Invalid Component | |
| status: stable | |
| props: | |
| type: object | |
| properties: | |
| test | |
| invalid: yaml | |
| slots: | |
| YAML | |
| ); | |
| $this->expectException(\Exception::class); | |
| $this->expectExceptionMessage('Component validation failed.'); | |
| try { | |
| $this->command->validateComponentDefinitions($this->testComponentsDir); | |
| } | |
| catch (\Exception $exception) { | |
| $output = $this->command->getOutputBuffer(); | |
| // Should show 1 valid component. | |
| $this->assertStringContainsString('✅ 1 components are valid', $output); | |
| // Should show the failure. | |
| $this->assertStringContainsString('Failed components:', $output); | |
| $this->assertStringContainsString('Mapping values are not allowed', $output); | |
| throw $exception; | |
| } | |
| } | |
| public function testMultipleComponentsWithFailures(): void { | |
| // Create valid component. | |
| file_put_contents( | |
| $this->testComponentsDir . '/valid.component.yml', | |
| <<<YAML | |
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | |
| name: Valid Component | |
| status: stable | |
| description: A valid component | |
| props: | |
| type: object | |
| slots: | |
| content: | |
| title: Content | |
| YAML | |
| ); | |
| // Create valid template. | |
| file_put_contents( | |
| $this->testComponentsDir . '/valid.twig', | |
| '{% block content %}{% endblock %}' | |
| ); | |
| // Create invalid component. | |
| file_put_contents( | |
| $this->testComponentsDir . '/invalid.component.yml', | |
| <<<YAML | |
| \$schema: https://git.drupalcode.org/project/drupal/-/raw/HEAD/core/assets/schemas/v1/metadata.schema.json | |
| name: Invalid Component | |
| status: stable | |
| props: | |
| type: object | |
| properties: | |
| test | |
| invalid: yaml | |
| slots: | |
| YAML | |
| ); | |
| // Create invalid template. | |
| file_put_contents( | |
| $this->testComponentsDir . '/invalid.twig', | |
| '{% block content %}{% endblock %}' | |
| ); | |
| $this->expectException(\Exception::class); | |
| $this->expectExceptionMessage('Component validation failed.'); | |
| try { | |
| $this->command->validateComponentDefinitions($this->testComponentsDir); | |
| } | |
| catch (\Exception $exception) { | |
| $output = $this->command->getOutputBuffer(); | |
| // Should show 1 valid component. | |
| $this->assertStringContainsString('✅ 1 components are valid', $output); | |
| // Should show the failure. | |
| $this->assertStringContainsString('Failed components:', $output); | |
| $this->assertStringContainsString('Mapping values are not allowed', $output); | |
| throw $exception; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php
around lines 195 to 243, the test creates two component YAML files but does not
create matching Twig template files, which causes the mixed success/failure
scenario and summary output to be incomplete; add creation of minimal template
files for both components in the test directory: after line ~210 create a file
named valid.component.twig with simple placeholder markup (e.g. a root element
or comment) and after line ~226 create invalid.component.twig similarly so the
validator sees corresponding templates for both components and the test can
assert the mixed results.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php (2)
63-68: Fix path sanitisation before deriving the component ID.
array_filter($component_path_parts);does nothing without reassigning the filtered result. With a trailing slash (common when tab-completing paths) the last segment becomes an empty string, so$component_base_identifierresolves to the wrong value, andfind()looks up the wrong component ID. The command then fails despite a valid directory.Reassign the filtered segments (and reindex) before using them:
- $component_path_parts = explode('/', $components_path); - array_filter($component_path_parts); + $component_path_parts = array_values(array_filter( + explode('/', $components_path), + static fn ($segment) => $segment !== '' + ));This guarantees the second-last element really is the extension machine name.
128-147: Do not overwrite the real component definition with hard-coded placeholders.
array_merge($definition, [...defaults...])replaces existing keys (provider,path,template,library, etc.) with stub values like'civictheme'and'foo.css'. That causesComponentValidatorto validate completely fabricated metadata, so valid components fail and broken components can appear to pass.The component plugin is already loaded on line 91 in
validateComponentDefinitions. Pass the component object to this method and reuse the definition Drupal already builds:Update the method signature and implementation:
- public function validateComponentFile(string $component_file, string $component_id): void { - [, $component_name] = explode(':', $component_id); - $definition = Yaml::parseFile($component_file); - // Merge with additional required keys. - $definition = array_merge( - $definition, - [ - 'machineName' => $component_name, - 'extension_type' => 'theme', - 'id' => $component_id, - 'library' => ['css' => ['component' => ['foo.css' => []]]], - 'path' => '', - 'provider' => 'civictheme', - 'template' => $component_name . '.twig', - 'group' => 'civictheme-group', - 'description' => 'CivicTheme component', - ] - ); + public function validateComponentFile(Component $component): void { + $definition = $component->getPluginDefinition(); $this->componentValidator->validateDefinition($definition, TRUE); }Then update the call on line 93:
- $this->validateComponentFile($component_file, $component_id); + $this->validateComponentFile($component);This keeps the validator aligned with the actual plugin metadata and works for any module/theme.
🧹 Nitpick comments (1)
web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php (1)
87-102: Consider more specific exception handling for better error diagnosis.The broad
\Exceptioncatch on line 96 will catch all exceptions, including unexpected fatal errors that should propagate. Consider catching specific exception types (InvalidComponentException,Error) separately to provide more targeted error messages and allow truly unexpected exceptions to fail fast during development.Example refactor:
foreach ($component_files as $component_file) { try { $component_name = basename((string) $component_file, '.component.yml'); $component_id = $component_base_identifier . ':' . $component_name; $component = $this->componentPluginManager->find($component_id); $this->validateSlots($component); $this->validateComponentFile($component_file, $component_id); $valid_count++; } + catch (InvalidComponentException $e) { + $errors[] = [ + 'file' => basename((string) $component_file), + 'error' => $e->getMessage(), + ]; + } + catch (Error $e) { + $errors[] = [ + 'file' => basename((string) $component_file), + 'error' => 'Twig error: ' . $e->getMessage(), + ]; + } catch (\Exception $e) { $errors[] = [ 'file' => basename((string) $component_file), - 'error' => $e->getMessage(), + 'error' => 'Unexpected error: ' . $e->getMessage(), ]; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php (1)
web/modules/custom/sdc_validator/tests/src/Kernel/ValidateComponentCommandTest.php (2)
__construct(260-263)output(268-270)
⏰ 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
| use Drupal\Core\Theme\Component\ComponentValidator; | ||
| use Drupal\Core\Theme\ComponentPluginManager; | ||
| use Drush\Commands\DrushCommands; | ||
| use Psy\Command\Command; |
There was a problem hiding this comment.
Replace incorrect Psy import with Consolidation.
The import Psy\Command\Command is incorrect for a Drush command. The exit codes used on lines 113 and 115 (Command::FAILURE and Command::SUCCESS) should come from Consolidation's AnnotatedCommand namespace.
Apply this diff to fix the import:
-use Psy\Command\Command;
+use Consolidation\AnnotatedCommand\CommandError;Then update the exit code references on lines 113 and 115:
- return CommandResult::dataWithExitCode('Component validation failed.', Command::FAILURE);
+ return CommandResult::dataWithExitCode('Component validation failed.', CommandError::getExitCode());
}
- return CommandResult::dataWithExitCode('✨ All components are valid', Command::SUCCESS);
+ return CommandResult::dataWithExitCode('✨ All components are valid', 0);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php
around line 15, replace the incorrect import "use Psy\Command\Command;" with
"use Consolidation\AnnotatedCommand\Command;" and ensure the exit code
references on lines 113 and 115 continue to use Command::FAILURE and
Command::SUCCESS (now coming from the Consolidation\AnnotatedCommand\Command
class).
| protected function validateSlots(Component $component): void { | ||
| $template_path = $component->getTemplatePath(); | ||
| if ($template_path === NULL) { | ||
| throw new \Exception(sprintf('❌ %s does not have a template.', $component->getI)); | ||
| } |
There was a problem hiding this comment.
Fix incomplete method call causing syntax error.
Line 173 has an incomplete method call $component->getI which will cause a fatal error at runtime. This should be $component->getId().
Apply this diff to fix the error:
- throw new \Exception(sprintf('❌ %s does not have a template.', $component->getI));
+ throw new \Exception(sprintf('❌ %s does not have a template.', $component->getId()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected function validateSlots(Component $component): void { | |
| $template_path = $component->getTemplatePath(); | |
| if ($template_path === NULL) { | |
| throw new \Exception(sprintf('❌ %s does not have a template.', $component->getI)); | |
| } | |
| protected function validateSlots(Component $component): void { | |
| $template_path = $component->getTemplatePath(); | |
| if ($template_path === NULL) { | |
| throw new \Exception(sprintf('❌ %s does not have a template.', $component->getId())); | |
| } |
🤖 Prompt for AI Agents
In web/modules/custom/sdc_validator/src/Commands/ValidateComponentCommand.php
around lines 170 to 174, there is an incomplete method call `$component->getI`
causing a syntax/fatal error; replace the incomplete call with the correct
`$component->getId()` so the exception message uses the component ID properly
and the code compiles/runs.
richardgaunt
added
State: Do not review
1 participant
JIRA ticket: #3549887
Checklist before requesting a review
Changed
Screenshots
Summary by CodeRabbit
New Features
Tests
Chores