Move ExecutionContext, ExecutionContextFactory, and ConstraintViolationBuilder from TypedData to Validation namespace

Postponed on 📌 Create an UploadedFile validator and deprecate error checking methods on UploadedFileInterface RTBC

Blocker is in

Marking as move seems fine.

Only concern is will this immediately break contrib modules? See some existing tests and validations had to be updated. But not sure how to backwards check a move.

We are subclassing the exact same code in a new namespace, so existing code should work fine, just trigger the deprecation warning. We could add a deprecation test, but we don't usually do that for constructors.

Added some questions to the MR

Addressed feedback.

appears all feedback has been addressed.

I asked anothe question on the MR

All threads resolved.

Seems all threads have been resolved again :)

We have an opportunity while doing this to add parameter and return typehints to everything that's missing them. (Normally, changes to moved code might be considered out of scope, but ensuring new API additions are typehinted properly is a higher priority and does not significantly impact the scope.) Once that's done, we can also mention it in the CR.

Thanks!

Some helpful test failures, mostly for strings vs. stringables.

I belatedly realized there might be some limitations in method typehinting based on the interface, but at least the constructors and member properties should be fair game.

Not sure what is causing the AdminUI Javascript test fails. 😒

I missed the string type errors in the kernel tests earlier. Finally found them!

I've added a deprecation trigger for \Drupal\Core\Validation\ConstraintViolationBuilder::atPath($path) because there were a couple of places passing int and the interface specifies string. Added this to the change record.

We are passing string, \Stringable, and int to \Drupal\Core\Validation\ConstraintViolationBuilder::setParameter(string $key, mixed $value). Not sure whether we should try and reduce that down to just string and \Stringable?

Move still seems fine

Added some feedback to the MR.

Reviewed the change and appears all feedback has been addressed.

Like we added typehints for everything too.

Deprecation appears correct, CR reads well.

LGTM

Needs a rebase.

Rebased on 11.x

Committed eacd1fa and pushed to 11.x. Thanks!
Committed 36f2784 and pushed to 10.3.x. Thanks!

diff --git a/core/lib/Drupal/Core/Validation/ExecutionContext.php b/core/lib/Drupal/Core/Validation/ExecutionContext.php
index ad732ea9f9..5fdf1e6e52 100644
--- a/core/lib/Drupal/Core/Validation/ExecutionContext.php
+++ b/core/lib/Drupal/Core/Validation/ExecutionContext.php
@@ -110,8 +110,12 @@ public function setConstraint(Constraint $constraint): void {
 
   /**
    * {@inheritdoc}
+   *
+   * phpcs:ignore Drupal.Commenting.FunctionComment.VoidReturn
+   *
+   * @return void
    */
-  public function addViolation(string $message, array $params = []): void {
+  public function addViolation(string $message, array $params = []) {
     $this->violations->add(new ConstraintViolation($this->translator->trans($message, $params, $this->translationDomain), $message, $params, $this->root, $this->propertyPath, $this->value, NULL, NULL, $this->constraint));
   }
 

Added the above code back to the 10.3.x backport so we didn't change return types.

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

kim.pepper → closed merge request !5104