Using ConstraintViolation::$arrayPropertyPath bugs on PHP 8.2

This is set in one place, from $violation->getPropertyPath():

          $property_path = explode('.', $violation->getPropertyPath());
          $delta = array_shift($property_path);
          if (is_numeric($delta)) {
            $violations_by_delta[$delta][] = $violation;
          }
          // Violations at the ItemList level are not associated to any delta.
          else {
            $item_list_violations[] = $violation;
          }
          $violation->arrayPropertyPath = $property_path;

This is used in two ways:

    if ($violation->arrayPropertyPath == ['format'] && isset($element['format']['#access']) && !$element['format']['#access']) {

    return ($element === FALSE) ? FALSE : $element[$violation->arrayPropertyPath[0]];

Instead of setting the property, I think we could just call $violation->getPropertyPath() again and recalculate what we need, something like:

    $property_path = explode('.', $violation->getPropertyPath());
    if ($property_path[1] == 'format' && isset($element['format']['#access']) && !$element['format']['#access']) {

    $property_path = explode('.', $violation->getPropertyPath());
    return ($element === FALSE) ? FALSE : $element[$property_path[1]];

Unsure what we can do about the uses in contrib, except write a change notice? We don't own the class that we are modifying :(

+++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWithSummaryWidget.php
@@ -100,7 +100,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    array_shift($property_path);
+    return ($element === FALSE) ? FALSE : $element[$property_path[0]];

Simpler to not call array_shift and use $property_path[1] instead here.

+++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWidget.php
@@ -46,7 +46,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+    array_shift($property_path);
+    if ($property_path == ['format'] && isset($element['format']['#access']) && !$element['format']['#access']) {

Even here I think we could just check $property_path[1] === 'format'?

+++ b/core/lib/Drupal/Core/Field/WidgetBase.php
@@ -449,7 +449,6 @@ public function flagErrors(FieldItemListInterface $items, ConstraintViolationLis
-          $violation->arrayPropertyPath = $property_path;

So the patch in #9 is good for core, but removing this line will break contrib, and I have no idea how we can notify them.

I tried to post a comment earlier but got lost due to crosspost, but I said essentially the same things.

Two ideas that I did have:

* Conditionally not set that property on PHP 8.2+, but that feels like it would just be more confusing.
* wrap the object with a decorator that triggers a deprecation (we could also just keep it then, but I feel like this isn't something we want to maintain long-term.

If it's not a final class we can sublass

ConstraintViolation isn't final so it'd be possible to subclass. It's possible that ConstraintViolationInterface gets implemented directly, but core at least is not doing this. A lot of private properties in there so I don't think we'd want to subclass permanently. Wondering if we want to class_alias so that the subclass itself doesn't become API surface that we then have to deprecate...

Change record + contrib issues sounds like a good plan to avoid a lot of complexity.

If we consider that, then see also my suggestion in #11:

> * Conditionally not set that property on PHP 8.2+, but that feels like it would just be more confusing.

If we would only stop doing that on PHP 8.2 then affected modules would have a bit more time to adapt and we wouldn't immediately break them with 10.0 assuming this lands there. It's kinda weird, but early adopters of 8.2 are probably going to test their sites more careful than others that just update Drupal?

Here's the same patch + Berdir's suggestion in #5/#17.

If we do this:

- Contrib modules will continue to work on PHP <= 8.1 with no changes
- We can open individual issues against contrib modules informing them of the change
- no tricky subclassing involved

It is a bit weird, but seems fine?

It is a bit weird, but seems fine?

PHPStan disagrees on the last part of that sentence.

harrumph.

- TestBot green
- Code changes make sense
- Concerns from @Berdir are addressed.
- Follow-up isssue with a @todo to remove this bc layer once PHP 8.2 is required are created.

The only thing that seems missing is:

We can open individual issues against contrib modules informing them of the change

Unsure if that should block this issue being committed, so I'm putting this on RTBC to get some core committer eyes on this one.

Thanks @andypost.

Added a change record.

This is a really odd one. One concern I have is that we'll not be able to do this on Drupal 9.5 and therefore that won't be able to be 100% PHP 8.2 compatible (or maybe even tested on PHP 8.2).

I'm not sure this is the correct fix. I think we could add the array property path to \Drupal\Core\Field\WidgetBase::errorElement() as this is the only place where this array can be used. This would resolve the @todo related to all this in the code...

  // @todo: Pass $violation->arrayPropertyPath as property path.
  $error_element = $this->errorElement($delta_element, $violation, $form, $form_state);

For BC we could implement a ViolationWrapper here that adds this property and triggers a deprecation when it is used to get this property. ViolationWrapper should be @internal and final. Or we could not set in PHP 8.2.

1. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -449,6 +449,9 @@ public function flagErrors(FieldItemListInterface $items, ConstraintViolationLis
   +        // @todo Remove BC layer https://www.drupal.org/i/3307859 on PHP 8.2.
   +        if (version_compare('8.2', phpversion())) {
              $violation->arrayPropertyPath = $property_path;
            }
   

   This isn't really a BC layer - because it is no longer working at all on PHP 8.2.

2. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
   @@ -609,4 +612,17 @@ protected function getFilteredDescription() {
   +  /**
   +   * Returns property path as array.
   +   *
   +   * @param \Symfony\Component\Validator\ConstraintViolationInterface $violation
   +   *   The violation to get its property path.
   +   *
   +   * @return array
   +   *   Array of path slugs.
   +   */
   +  protected function getPropertyPathArray(ConstraintViolationInterface $violation): array {
   +    return explode('.', $violation->getPropertyPath());
   +  }
   
   +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWidget.php
   @@ -46,7 +46,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    $property_path = $this->getPropertyPathArray($violation);
   +    array_shift($property_path);
   +    if ($property_path == ['format'] && isset($element['format']['#access']) && !$element['format']['#access']) {
   

   Why is the method not doing the array shift. Also this method is in a bit of an odd place. Maybe a better name like getPropertyPathArrayFromViolation() would make the location make more sense.

3. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWithSummaryWidget.php
   @@ -100,7 +100,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -    return ($element === FALSE) ? FALSE : $element[$violation->arrayPropertyPath[0]];
   +    $property_path = $this->getPropertyPathArray($violation);
   +    return ($element === FALSE) ? FALSE : $element[$property_path[1]];
   
   +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextfieldWidget.php
   @@ -37,7 +37,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   -    if ($violation->arrayPropertyPath == ['format'] && isset($element['format']['#access']) && !$element['format']['#access']) {
   +    $property_path = $this->getPropertyPathArray($violation);
   +    array_shift($property_path);
   +    if ($property_path == ['format'] && isset($element['format']['#access']) && !$element['format']['#access']) {
   

   These changes mean that we're now doing a more expensive check first and then cheap checks in the if. I'm not sure that that's a good idea.

The errorElement() change seems sensible, completely missed that reviewing this issue.

For the bc layer, I am still not sure, but I guess given we only set the property from within WidgetBase, we could only wrap it from within WidgetBase - in any other context it won't have that property set anyway. That might mean we can make the decorator a bit lighter than it'd need to be otherwise.

Alternatively we don't provide any replace API at all... and do something like the patch attached...

Will need to add some tests for \Drupal\Core\Field\InternalViolation...

No interdiff because this is an alternate approach.

Here's the patch fixed and with a test. There is test coverage of the filter format access stuff in core/modules/filter/tests/src/Functional/FilterFormatAccessTest.php - if you revert the changes to the text module and run this test you get the expected deprecations.

Yes, that's exactly what I was thinking originally and is nicer than just breaking it on PHP 8.2. Didn't review in detail yet, but +1 on the direction.

Here's #32 combined with the errorElement change (in a BC compatible way). Adding arguments to interface methods is icky and I don't think we've come up with a process yet. I've borrowed the approach from Symfony. Looking at the use-cases from #15's link I can see an advantage of preserving this information and passing it to the each widget's errorElement method. But we're not good at this sort of API change because it is not easy. However I believe this is what was envisaged by the original comment that's removed. I guess we did this using a dynamic property original to avoid late API change in the Drupal 8 release.

I think #32 is enough to solve the immediate problem at hand, we could leave the @todo in place for now and open a followup to solve it separately later? As #34 states we don't really have a policy for handling this kind of interface change yet, and while we probably will end up following Symfony maybe we should discuss that first, otherwise we're just swapping one @todo for another (the removal of the commented out method parameter).

+++ b/core/lib/Drupal/Core/Field/InternalViolation.php
@@ -0,0 +1,139 @@
+
+  /**
+   * {@inheritdoc}
+   */
+  public function __set(string $name, $value): void {
+    if ($name === 'arrayPropertyPath') {
+      $this->arrayPropertyPath = $value;
+      return;
+    }

I don't know if it's happening anywhere, but it seems possible that a subclass of WidgetBase is setting arrayPropertyPath. Given that, is it worth adding a setArrayPropertyPath() that's @internal and doesn't trigger a deprecation message, then we could always trigger a deprecation message here to catch those cases too.

By the letter of the deprecation policy the only way we'd be able to do this is to add a new method with the signature we want and deprecate the old one, which is going to be more disruptive than adding the parameter the Symfony way probably.

I quite like the idea of #35, but if we haven't got the new API in place, we can't deprecate the old API, so it'd mean committing the new shim with no deprecations (or maybe just on __set()) and then deprecating in the follow-up. That might be fine though.

Re #36 we could even set this in the constructor for extra neatness. Nice idea.

Re #39 I think we have to support setting any old random stuff of the class still and once we have a __get and __set we need to handle the deprecations ourselves.

Here's #36 implemented.

Re #35 if we don't change the method signature / calling arguments here then we'll never do it because new implementations will have to use

$property_path = explode('.', $violation->getPropertyPath());
array_shift($property_path);

To get the same array.

Re #41 if downstream users can already get the data from the violation object why do we need to separately add it to the API as well? I guess I'm questioning whether the @todo is valid in the first place. Most implementations of errorElement don't seem to need it, it's only used for a single special case.

@longwave no API would look like #3298731-31: Using ConstraintViolation::$arrayPropertyPath bugs on PHP 8.2. If we did that we'd want 'deprecated with no replacement, see change record on what to do instead' sort of messaging in the deprecation layer. That would have us having to deal with adding parameters to interface methods then at least.

Re #42 it is perfect possible to add a property to a validator like we are doing - we have to continue to support that while deprecating it. If we don't have that code we'd break it. See the tests. For example a module could implement a widget that ::errorElement and also have a base class that implements ::errorElement too and it could stuff public properties on the violation exactly like core's widgetBase::flagErrors() already does.

I'm sympathetic to #43. There's part of me that wonders if we shouldn't actually add a wrapper here permanently - to pass this information along - we could even change the interface from
\Drupal\Core\Field\WidgetInterface::errorElement(array $element, ConstraintViolationInterface $violation, array $form, FormStateInterface $form_state);
to
\Drupal\Core\Field\WidgetInterface::errorElement(array $element, ConstraintViolationWrapper $violation, array $form, FormStateInterface $form_state);
gradually. And the only thing we'd be deprecating is public dynamic property access.

So should we go back to #32? That seems the simplest route out of here for now, and we can either keep the wrapper and extend it or just drop it in Drupal 11. @catch's comment in #37 is bypassed if we don't actually need to add any API, the change record can just tell you how to get the value from the violation object itself.

Re-uploading #32.

If we agree on this approach then the change record needs updating to match.

Code is looking good except for a personal nit. Also i have one question:

+++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWithSummaryWidget.php
@@ -100,7 +100,8 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
-    return ($element === FALSE) ? FALSE : $element[$violation->arrayPropertyPath[0]];

Am I missing something here? Why would using explode mean the the element we need changes to the second part of the property instead of the first of the array?

1. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextareaWidget.php
   @@ -46,7 +46,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    if (isset($element['format']['#access']) && !$element['format']['#access'] && preg_match('/^[0-9]*\.format$/', $violation->getPropertyPath())) {
   

   Personally I dont like this pattern !$element['format']['#access'], but is a personal nit I think.

2. +++ b/core/modules/text/src/Plugin/Field/FieldWidget/TextfieldWidget.php
   @@ -37,7 +37,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    if (isset($element['format']['#access']) && !$element['format']['#access'] && preg_match('/^[0-9]*\.format$/', $violation->getPropertyPath())) {
   

   Same here

Am I missing something here? Why would using explode mean the the element we need changes to the second part of the property instead of the first of the array?

Equally puzzled about this … and scarier still: this is passing tests!? 😅

Because the old property already cut of the first part and the original one doesn't.

@Berdir: could you elaborate? 🤞 I'm not quite grokking this. I don't see any cutting? (It's been a long day so I might just be misinterpreting/overlooking 🙈)

Oh wow. Sorry i missed that, but i dont feel bad since @Wim Leers felt the same :P

My nits are just that, nits.

So, i'd RTBC but the change record needs updating.

• Thanks @andypost 😅
• @bbrala's nits in #50 are both pre-existing. So IMHO out of scope to change them here.
• Change record updated. 👍
• Queued a PHP 8.2-RC1 test run of #48. Assuming that comes back green, I'll RTBC.
• Review:
  

  +++ b/core/tests/Drupal/Tests/Core/Field/InternalViolationTest.php
  @@ -0,0 +1,38 @@
  +    $this->expectDeprecation('Setting dynamic properties on violations is deprecated in drupal:9.5.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3307919');
  +    $this->expectDeprecation('Accessing dynamic properties on violations is deprecated in drupal:9.5.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3307919');
  +    $violation->foo = 'bar';
  +    $this->assertSame('bar', $violation->foo);
  

  TIL you can test two deprecations in a single method 😅

  (I ran & debugged this locally to convince myself 🙈)

> Queued a PHP 8.2-RC1 test run of #48. Assuming that comes back green, I'll RTBC.

There are still 5+ open issues and plenty of problems, that doesn't really make sense. This patch is already being tested in #3295821: Ignore: patch testing issue for PHP 8.2 attributes

#58: Ah … I did not realize that. I've only been told by @xjm that this is specific issue is a de facto Drupal 10 beta blocker 🤷

Added one little paragraph to the CR (thanks for ninja editing grrr).

Please note, the new array getPropertyPath returns is keyed slightly different. The new code does not shift an element off the start of the array. Therefor to get the same value as before you NEED to add 1 to the key as shown in the example.

I fully reviewed this and the CR. All lights are green imo

I uploaded a couple of patches here but not involving any changes that actually made it into the final patch, so I think I'm still OK to commit.

Committed/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!

Published the change record.

The MR title is:

> New ::getPropertyPathArray() method on Drupal\Core\Field\WidgetBase

but the text is about a new method ConstraintViolationInterface::getPropertyPath()

I'm not sure which is right. Could someone fix it?

Thanks @joachim, updated the CR title to "The undeclared ConstraintViolation::$arrayPropertyPath property is deprecated".

> Please note, the array getPropertyPath returns is keyed slightly different if you explore the path. The new code does not shift an element off the start of the array. Therefor to get the same value as before you NEED to add 1 to the key as shown in the example.

FWIW, the removal is dynamic because not all widgets have a delta, checkbox/boolean fields specifically do not, that's why it's only removed if there and for them, nothing changes. But that's very much an edge case.

Ok, there's more wrong/confusing about the CR.

The InternalViolation class exists for the sole purpose of providing BC to existing code, it IMHO doesn't need to be mentioned at all, it's magic and will magically disappear again in D11.

getPropertyPath is a method of ConstraintViolationInterface and isn't new, we just have to implement it in our new temporary internal class like all others.

Ok, thank you for having a look. I'll revert to the simple version but with the corrected method name when I get back to a pc.

Doing many small things while juggling kids is a bit confusing, thanks again @Berdir. Simplified the CR.

Well, that is awkward. @Spokje just noticed this breaks on php 7.4 because of union type.

public function getMessage(): string | \Stringable {

Good spot. Not sure a return type would help anything here, therefore for me #72 is RTBC.

Committed/pushed to 9.5.x, thanks!