-
Notifications
You must be signed in to change notification settings - Fork 20
[#3549887] Added additional component validation #1430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| services: | ||
| sdc_validator.commands: | ||
| class: \Drupal\sdc_validator\Commands\ValidateComponentCommand | ||
| arguments: ['@plugin.manager.sdc', '@twig'] | ||
| tags: | ||
| - { name: drush.command } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
| xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.5/phpunit.xsd" | ||
| bootstrap="../build/web/core/tests/bootstrap.php" | ||
| colors="true" | ||
| cacheResultFile="../build/.phpunit.cache/test-results" | ||
| executionOrder="depends,defects" | ||
| beStrictAboutOutputDuringTests="true" | ||
| beStrictAboutTodoAnnotatedTests="true" | ||
| convertWarningsToExceptions="false" | ||
| failOnRisky="true" | ||
| failOnWarning="false" | ||
| verbose="true"> | ||
| <testsuites> | ||
| <testsuite name="sdc_validator"> | ||
| <directory>./tests/src/</directory> | ||
| </testsuite> | ||
| </testsuites> | ||
| <php> | ||
| <ini name="error_reporting" value="-1" /> | ||
| <server name="KERNEL_DIR" value="../build/web/core" /> | ||
| <env name="SIMPLETEST_BASE_URL" value="http://localhost" /> | ||
| <env name="SIMPLETEST_DB" value="sqlite://localhost/sites/default/files/.ht.sqlite" /> | ||
| </php> | ||
| </phpunit> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| name: Single Directory Component Validator | ||
| type: module | ||
| description: 'Validator for Single Directory components.' | ||
| package: Custom | ||
| core_version_requirement: ^10 || ^11 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| <?php | ||
|
|
||
| /** | ||
| * @file | ||
| */ | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * @file | ||
| * CivicTheme Development module. | ||
| */ | ||
|
|
||
| /** | ||
| * Implements hook_twig_validator_rule_info_alter(). | ||
| */ | ||
| function sdc_validator_twig_validator_rule_info_alter(array &$info): void { | ||
| // @see https://www.drupal.org/project/sdc_devel/issues/3517321 | ||
| $rule_name_ignore = -1; | ||
| $rule_name_error = 3; | ||
| $rule_name_warning = 4; | ||
| $rule_name_notice = 5; | ||
|
|
||
| $info['filter']['rule_on_name'][$rule_name_ignore][] = 'raw'; | ||
|
|
||
| // Ignore "Use slots instead of hard embedding a component in the template with `@name`.". | ||
| unset($info['include']); | ||
| // Ignore "Use slots instead of hard embedding a component in the template with `includ\r\ne`.". | ||
| unset($info['function']['rule_on_name'][$rule_name_error]['parent']); | ||
| // Ignore "Replace with Twig function include()". | ||
| unset($info['function']['rule_on_name'][$rule_name_notice]['pattern']); | ||
| // Ignore "Careful with Twig function: `source`. Bad architecture, but sometimes needed\r\n for shared static files.". | ||
| $info['function']['rule_on_name'][$rule_name_ignore][] = 'source'; | ||
| unset($info['function']['rule_on_name'][$rule_name_warning]['source']); | ||
| // Ignore "`is iterable` test is too ambiguous. Use `is sequence` or `is mapping`.". | ||
| unset($info['test']); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,222 @@ | ||||||||||||||||||||||||||||||
| <?php | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| declare(strict_types=1); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| namespace Drupal\sdc_validator\Commands; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| use Consolidation\AnnotatedCommand\CommandResult; | ||||||||||||||||||||||||||||||
| use Drupal\Core\Plugin\Component; | ||||||||||||||||||||||||||||||
| use Drupal\Core\Render\Component\Exception\InvalidComponentException; | ||||||||||||||||||||||||||||||
| use Drupal\Core\Template\ComponentNodeVisitor; | ||||||||||||||||||||||||||||||
| use Drupal\Core\Template\TwigEnvironment; | ||||||||||||||||||||||||||||||
| use Drupal\Core\Theme\Component\ComponentValidator; | ||||||||||||||||||||||||||||||
| use Drupal\Core\Theme\ComponentPluginManager; | ||||||||||||||||||||||||||||||
| use Drush\Commands\DrushCommands; | ||||||||||||||||||||||||||||||
| use Psy\Command\Command; | ||||||||||||||||||||||||||||||
| use Symfony\Component\Yaml\Yaml; | ||||||||||||||||||||||||||||||
| use Twig\Error\Error; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Validates SDC component definitions using Drupal core's ComponentValidator. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @package Drupal\sdc_validator\Commands | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| class ValidateComponentCommand extends DrushCommands { | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Defines the component validator. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| protected ComponentValidator $componentValidator; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Validates slots of a component. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| protected ComponentNodeVisitor $componentSlotValidator; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * {@inheritdoc} | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| public function __construct(protected ComponentPluginManager $componentPluginManager, protected TwigEnvironment $twig) { | ||||||||||||||||||||||||||||||
| parent::__construct(); | ||||||||||||||||||||||||||||||
| $this->componentValidator = new ComponentValidator(); | ||||||||||||||||||||||||||||||
| $this->componentValidator->setValidator(); | ||||||||||||||||||||||||||||||
| $this->componentSlotValidator = new ComponentNodeVisitor($this->componentPluginManager); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Validates all component definitions in a given path. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @param string $components_path | ||||||||||||||||||||||||||||||
| * Path to the directory containing component folders. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @command sdc_validator:validate | ||||||||||||||||||||||||||||||
| * @usage drush sdc_validator:validate '<path to components>' | ||||||||||||||||||||||||||||||
| * @usage drush sdc_validator:validate 'web/themes/custom/civictheme/components' | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @SuppressWarnings(PHPMD.StaticAccess) | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| public function validateComponentDefinitions(string $components_path): CommandResult { | ||||||||||||||||||||||||||||||
| if (!is_dir($components_path)) { | ||||||||||||||||||||||||||||||
| throw new \Exception('❌ Components directory not found: ' . $components_path); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| $this->output()->writeln(sprintf('🔍 Validating components in %s', $components_path)); | ||||||||||||||||||||||||||||||
| $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); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+63
to
+68
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix path sanitisation before deriving the component ID.
- $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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| $component_files = []; | ||||||||||||||||||||||||||||||
| $iterator = new \RecursiveIteratorIterator( | ||||||||||||||||||||||||||||||
| new \RecursiveDirectoryIterator($components_path), | ||||||||||||||||||||||||||||||
| \RecursiveIteratorIterator::SELF_FIRST | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| foreach ($iterator as $file) { | ||||||||||||||||||||||||||||||
| if ($file->isFile() && $file->getExtension() === 'yml' && str_ends_with((string) $file->getFilename(), '.component.yml')) { | ||||||||||||||||||||||||||||||
| $component_files[] = $file->getPathname(); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| if (empty($component_files)) { | ||||||||||||||||||||||||||||||
| throw new \Exception('❌ No component definition files found in: ' . $components_path); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| $errors = []; | ||||||||||||||||||||||||||||||
| $valid_count = 0; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| 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 (\Exception $e) { | ||||||||||||||||||||||||||||||
| $errors[] = [ | ||||||||||||||||||||||||||||||
| 'file' => basename((string) $component_file), | ||||||||||||||||||||||||||||||
| 'error' => $e->getMessage(), | ||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Display summary. | ||||||||||||||||||||||||||||||
| if ($valid_count > 0) { | ||||||||||||||||||||||||||||||
| $this->output()->writeln(sprintf("✅ %d components are valid", $valid_count)); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if ($errors !== []) { | ||||||||||||||||||||||||||||||
| $this->output()->writeln("Failed components:"); | ||||||||||||||||||||||||||||||
| foreach ($errors as $error) { | ||||||||||||||||||||||||||||||
| $this->output()->writeln(sprintf("❌ %s - %s", $error['file'], $error['error'])); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| return CommandResult::dataWithExitCode('Component validation failed.', Command::FAILURE); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| return CommandResult::dataWithExitCode('✨ All components are valid', Command::SUCCESS); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Validates a single component definition file. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @param string $component_file | ||||||||||||||||||||||||||||||
| * Path to the file. | ||||||||||||||||||||||||||||||
| * @param string $component_id | ||||||||||||||||||||||||||||||
| * The component id. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @throws \Drupal\Core\Render\Component\Exception\InvalidComponentException | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| 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', | ||||||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| $this->componentValidator->validateDefinition($definition, TRUE); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Moved from \Drupal\Core\Template\ComponentNodeVisitor::validateSlots. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * Performs a cheap validation of the slots in the template. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * It validates them against the JSON Schema provided in the component | ||||||||||||||||||||||||||||||
| * definition file and massaged in the ComponentMetadata class. We don't use | ||||||||||||||||||||||||||||||
| * the JSON Schema validator because we just want to validate required and | ||||||||||||||||||||||||||||||
| * undeclared slots. This cheap validation lets us validate during runtime | ||||||||||||||||||||||||||||||
| * even in production. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @param \Drupal\Core\Plugin\Component $component | ||||||||||||||||||||||||||||||
| * The component to validate the slots against. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @throws \Drupal\Core\Render\Component\Exception\InvalidComponentException | ||||||||||||||||||||||||||||||
| * When the twig doesn't parse or template does not exist. | ||||||||||||||||||||||||||||||
| * @throws \Exception | ||||||||||||||||||||||||||||||
| * When the slots don't pass validation. | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * @see \Drupal\Core\Template\ComponentNodeVisitor::validateSlots | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| 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)); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
Comment on lines
+170
to
+174
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix incomplete method call causing syntax error. Line 173 has an incomplete method call 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||
| $source = $this->twig->getLoader()->getSourceContext($template_path); | ||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||
| // Need to load as a component. | ||||||||||||||||||||||||||||||
| $node_tree = $this->twig->parse($this->twig->tokenize($source)); | ||||||||||||||||||||||||||||||
| $node = $node_tree->getNode('blocks'); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| catch (Error $error) { | ||||||||||||||||||||||||||||||
| throw new \Exception("❌ Error parsing twig file: " . $error->getMessage(), $error->getCode(), $error); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| $metadata = $component->metadata; | ||||||||||||||||||||||||||||||
| if (!$metadata->mandatorySchemas) { | ||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| $slot_definitions = $metadata->slots; | ||||||||||||||||||||||||||||||
| $ids_available = array_keys($slot_definitions); | ||||||||||||||||||||||||||||||
| $undocumented_ids = []; | ||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||
| $it = $node->getIterator(); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| catch (\Exception) { | ||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if ($it instanceof \SeekableIterator) { | ||||||||||||||||||||||||||||||
| while ($it->valid()) { | ||||||||||||||||||||||||||||||
| $provided_id = $it->key(); | ||||||||||||||||||||||||||||||
| if (!in_array($provided_id, $ids_available, TRUE)) { | ||||||||||||||||||||||||||||||
| $undocumented_ids[] = $provided_id; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| $it->next(); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| // Now build the error message. | ||||||||||||||||||||||||||||||
| $error_messages = []; | ||||||||||||||||||||||||||||||
| if (!empty($undocumented_ids)) { | ||||||||||||||||||||||||||||||
| $error_messages[] = sprintf( | ||||||||||||||||||||||||||||||
| 'We found an unexpected slot that is not declared: [%s]. Declare them in "%s.component.yml".', | ||||||||||||||||||||||||||||||
| implode(', ', $undocumented_ids), | ||||||||||||||||||||||||||||||
| $component->machineName | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| if (!empty($error_messages)) { | ||||||||||||||||||||||||||||||
| $message = implode("\n", $error_messages); | ||||||||||||||||||||||||||||||
| throw new InvalidComponentException($message); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace incorrect Psy import with Consolidation.
The import
Psy\Command\Commandis incorrect for a Drush command. The exit codes used on lines 113 and 115 (Command::FAILUREandCommand::SUCCESS) should come from Consolidation'sAnnotatedCommandnamespace.Apply this diff to fix the import:
Then update the exit code references on lines 113 and 115:
🤖 Prompt for AI Agents