Updated: Comment #0

Problem/Motivation

Follow-up from #2002158-61: Convert form validation of comments to entity validation
Constraint validators are unable to use ContainerFactoryPluginInterface because they are created using a new $class in Symfony's ConstraintValidatorFactory

Proposed resolution

Extend symfony's ConstraintValidatorFactory and include the logic for ContainerFactoryPluginInterface

Remaining tasks

None

User interface changes

none

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because leads to better code, allowing DI
Issue priority Normal because nicer DX - note was raised in review of #2403817: Feed entity validation misses form validation logic which is critical
Disruption None
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Fixed

Well, we still use the default factory here:

class ConstraintManager extends DefaultPluginManager {

  /**
   * Overrides \Drupal\Component\Plugin\PluginManagerBase::__construct().
   *
   * @param \Traversable $namespaces
   *   An object that implements \Traversable which contains the root paths
   *   keyed by the corresponding namespace to look for plugin implementations.
   * @param \Drupal\Core\Cache\CacheBackendInterface $cache_backend
   *   Cache backend instance to use.
   * @param \Drupal\Core\Language\LanguageManager $language_manager
   *   The language manager.
   * @param \Drupal\Core\Extension\ModuleHandlerInterface $module_handler
   *   The module handler to invoke the alter hook with.
   */
  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, LanguageManager $language_manager, ModuleHandlerInterface $module_handler) {
    parent::__construct('Plugin/Validation/Constraint', $namespaces);
    $this->discovery = new StaticDiscoveryDecorator($this->discovery, array($this, 'registerDefinitions'));
    $this->alterInfo($module_handler, 'validation_constraint');
    $this->setCacheBackend($cache_backend, $language_manager, 'validation_constraint_plugins');
  }

  /**
   * Creates a validation constraint.
   *
   * @param string $name
   *   The name or plugin id of the constraint.
   * @param mixed $options
   *   The options to pass to the constraint class. Required and supported
   *   options depend on the constraint class.
   *
   * @return \Symfony\Component\Validator\Constraint
   *   A validation constraint plugin.
   */
  public function create($name, $options) {
    if (!is_array($options)) {
      // Plugins need an array as configuration, so make sure we have one.
      // The constraint classes support passing the options as part of the
      // 'value' key also.
      $options = array('value' => $options);
    }
    return $this->createInstance($name, $options);
  }

The described class is just for the symfony part of the constrains.

andypost’s picture

Status: Fixed » Active

I still think this needs more investigation, constraints could become more complex and better to inject their dependencies

Xano’s picture

Status: Active » Closed (cannot reproduce)

ConstraintsManager uses ContainerFactory, and not ConstraintValidationFactory (which belongs to Symfony), so dependency injection works.

If this doesn't answer your question, then I'm afraid I don't know what you mean.

larowlan’s picture

Status: Closed (cannot reproduce) » Active

Yes, constraint plugins can implement ContainerFactoryPluginInterface, but validators cannot.

See CommentNameConstraintValidator.

Also see #2403817: Feed entity validation misses form validation logic - the protected function feedStorage() over there demonstrates that ContainerFactoryPluginInterface doesn't work for the validators.

larowlan’s picture

Status: Active » Needs review
FileSize
2.19 KB

And an example/failing test

larowlan’s picture

And a fix showing it working

The last submitted patch, 5: constraints-damnit-2197029.fail_.patch, failed testing.

larowlan’s picture

And remove the comment that sent me here.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks great

larowlan’s picture

Issue summary: View changes

beta eval, issue update

dawehner’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Validation/ConstraintValidatorFactory.php
@@ -0,0 +1,42 @@
+      if (is_subclass_of($class_name, 'Drupal\Core\Plugin\ContainerFactoryPluginInterface')) {
+        $this->validators[$class_name] = $class_name::create(\Drupal::getContainer(), [], NULL, []);
+      }

Passing along the plugin configuration/ID and definition is pointless afaik. Why do you not just use the ClassResolver?

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.26 KB
5.27 KB

Adapted the code a bit.

larowlan’s picture

Nice

Status: Needs review » Needs work

The last submitted patch, 12: 2197029-12.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
497 bytes
7.28 KB

meh.

Status: Needs review » Needs work

The last submitted patch, 15: 2197029-15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.23 KB
1.44 KB

Let's fix it.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

let's do it

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2197029-17.patch, failed testing.

larowlan queued 17: 2197029-17.patch for re-testing.

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community

better :)

fago’s picture

This looks good, however I'm unsure how people will know they can do it that way? Should we document it at the manager given #2195083: Add a dedicated @Constraint annotation class is not done yet.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2197029-17.patch, failed testing.

larowlan’s picture

Status: Needs work » Reviewed & tested by the community

random fails

larowlan queued 17: 2197029-17.patch for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

2197029-17.patch no longer applies.

error: patch failed: core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php:7
error: core/modules/comment/src/Plugin/Validation/Constraint/CommentNameConstraintValidator.php: patch does not apply

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
7.31 KB

reroll

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Straight reroll

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I think being able to unit test validators cleanly reduces fragility and there is no disruption. Committed 5f7ec33 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 5f7ec33 on 8.0.x
    Issue #2197029 by dawehner, larowlan, ParisLiakos: Allow to inject...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.