Problem/Motivation

Drupal validation constraint plugins simply extend Symfony constraint plugins, e.g.

class IsNullConstraint extends IsNull {}

This means that we inherit the constructor from Symfony.

ConstraintManager uses the default ContainerFactory to construct plugin instances:

    return new $plugin_class($configuration, $plugin_id, $plugin_definition);

Technically this is compatible with the Constraint constructor in Symfony 4:

    public function __construct($options = null)

However in Symfony 5, two new parameters have been added:

    public function __construct($options = null, array $groups = null, $payload = null)

This fails, because $plugin_id is a string but we try to pass it as the $groups array parameter.

Steps to reproduce

Upgrade symfony/validator to 5.2 and try to run core tests.

Proposed resolution

Add a ConstraintFactory that calls the constructor correctly.

Remaining tasks

Extract part of the patch from #3161889-67: [META] Symfony 6 compatibility
Add tests

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3185603-11.patch6.54 KBdaffie
#13 interdiff-3185603-9-11.txt1.37 KBdaffie
#9 interdiff-3185603-7-9.txt4.17 KBdaffie
#9 3185603-9.patch6.56 KBdaffie
#7 3185603-7.patch2.39 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Issue summary: View changes
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Title: Symfony Constraint plugins are not strictly Drupal plugins » [Symfony 6] Symfony Constraint plugins are not strictly Drupal plugins
Issue tags: +Symfony 6
Parent issue: » #3161889: [META] Symfony 6 compatibility

Good find! Tagging up.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: [Symfony 6] Symfony Constraint plugins are not strictly Drupal plugins » [Symfony 5] Symfony Constraint plugins are not strictly Drupal plugins
Issue tags: -Symfony 6 +Symfony 5
Parent issue: #3161889: [META] Symfony 6 compatibility » #3055180: [META] Symfony 5 compatibility
Related issues: +#3161889: [META] Symfony 6 compatibility

We actually need to solve this before Symfony 6, as this will break in 5.2 or above. Symfony constructors do have BC, but we were always calling this one incorrectly, and the extra arguments were simply being ignored.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: +Symfony 6

We used the Symfony 5 tag for things that break when updating to Symfony 5.0. Let's at least add the Symfony 6 tag.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Looks it caused by PHP 8 additions https://symfony.com/blog/new-in-symfony-5-2-constraints-as-php-attributes

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
3185603-7.patch2.39 KB

Slightly improved version of the factory from the other issue, this checks whether we really are creating a Constraint and falls back in the unlikely case that we have a normal Drupal plugin.

This probably needs a test at least for the Symfony case.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Priority: Normal » Critical
Status: Needs review » Needs work
Issue tags: +Drupal 10

Since this blocks Drupal 10 it should be marked critical. Approach looks good but agreed we need tests here.

daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3185603-9.patch6.56 KB
interdiff-3185603-7-9.txt4.17 KB

Added testing.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Tests look good, one question:

+++ b/core/tests/Drupal/KernelTests/Core/Validation/ConstraintFactoryTest.php
@@ -0,0 +1,48 @@
+    $this->assertNotInstanceOf(Constraint::class, $container_factory_plugin);
+    $this->assertTrue(is_subclass_of($container_factory_plugin, ContainerFactoryPluginInterface::class));
...
+    $this->assertNotInstanceOf(Constraint::class, $default_plugin);
+    $this->assertFalse(is_subclass_of($default_plugin, ContainerFactoryPluginInterface::class));
+    $this->assertInstanceOf(PluginBase::class, $default_plugin);

Why is_subclass_of() and not assert[Not]InstanceOf()? Aren't they equivalent here?

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented 
+++ b/core/lib/Drupal/Core/Validation/ConstraintFactory.php
@@ -0,0 +1,38 @@
+    // If the plugin provides a factory method, pass the container to it.
+    if (is_subclass_of($plugin_class, ContainerFactoryPluginInterface::class)) {
+      return $plugin_class::create(\Drupal::getContainer(), $configuration, $plugin_id, $plugin_definition);
+    }
+
+    // If the plugin is a Symfony Constraint, use the correct constructor.
+    if (is_subclass_of($plugin_class, Constraint::class)) {
+      return new $plugin_class($configuration);
+    }

What happens if a developer creates a plugin that does both?

Should the plugin manager catch this during discovery and throw an exception?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

I think it should still work if you want a Symfony Constraint that also needs the Drupal container? It would be up to the ::create() method defined in the plugin to instantiate the plugin correctly via the constructor.

daffie’s picture

daffie CreditAttribution: daffie commented
FileSize
interdiff-3185603-9-11.txt1.37 KB
3185603-11.patch6.54 KB

@longwave: You are right.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thanks! This looks great now, assuming tests come back green.

  • catch committed d0e54c0 on 9.2.x 
    Issue #3185603 by daffie, longwave, joachim: [Symfony 5] Symfony...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed d0e54c0 and pushed to 9.2.x. Thanks!

This probably could be backported to 9.1.x, but I don't see any reason to do so since it won't become a problem until we actually upgrade to Symfony 5/6, so leaving in 9.1.x

Status: Fixed » Closed (fixed)

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