[PropertyAccess][Serializer] Fix "type unknown" on denormalize

[seferov]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
License	MIT

The bug can clearly be seen on tests for RequestPayloadValueResolver on https://github.com/symfony/symfony/blob/6.3/src/Symfony/Component/HttpKernel/Tests/Controller/ArgumentResolver/RequestPayloadValueResolverTest.php#L255

// ...
  public function testValidationNotPassed()
  {
      $content = '{"price": 50, "title": ["not a string"]}';
      $payload = new RequestPayload(50);
      $serializer = new Serializer([new ObjectNormalizer()], ['json' => new JsonEncoder()]);
  
      // removed for simplification
  
      try {
          $resolver->onKernelControllerArguments($event);
          $this->fail(sprintf('Expected "%s" to be thrown.', HttpException::class));
      } catch (HttpException $e) {
          $validationFailedException = $e->getPrevious();
          $this->assertInstanceOf(ValidationFailedException::class, $validationFailedException);
          $this->assertSame('This value should be of type unknown.', $validationFailedException->getViolations()[0]->getMessage());
          $this->assertSame('Test', $validationFailedException->getViolations()[1]->getMessage());
      }
  }
}

class RequestPayload
{
    #[Assert\Length(min: 10, groups: ['strict'])]
    public string $title;

    public function __construct(public readonly float $price)
    {
    }
}

Even though title is string, the error message shown to user is ambiguous: "This value should be of type unknown.". Instead, the message should be "This value should be of type string."

[yceruto]

Even if it looks like a bug, I have doubts about whether this should go to 7.1 as an improvement.

[nicolas-grekas]

Even if it looks like a bug, I have doubts about whether this should go to 7.1 as an improvement.

I agree with this sentiment. Technically it's a new feature because it adds a new class so let's target 7.1.

[yceruto]

LGTM, Thank you Farhad!

[xabbuh]

The low deps failures are related. We need to bump the dev requirement on symfony/property-access to ^7.1.

[xabbuh]

the composer.json file to update is located in src/Symfony/Component/HttpKernel

[seferov]

the composer.json file to update is located in src/Symfony/Component/HttpKernel

@xabbuh correct me if I am wrong but these unit tests on HttpKernel component runs with Serializer & ProperyAccess with version 7.1.x-dev, not with the code that are on the PR. That's why they fail.

[xabbuh]

@seferov Can you please rebase your changes on the latest 7.1 branch?

[seferov]

@seferov Can you please rebase your changes on the latest 7.1 branch?

@xabbuh done

[xabbuh]

@xabbuh correct me if I am wrong but these unit tests on HttpKernel component runs with Serializer & ProperyAccess with version 7.1.x-dev, not with the code that are on the PR.

In fact, our pipeline is a bit more complex and takes the changes from the PR into account: https://github.com/symfony/symfony/blob/7.1/.github/workflows/unit-tests.yml#L91

[seferov]

In fact, our pipeline is a bit more complex and takes the changes from the PR into account: https://github.com/symfony/symfony/blob/7.1/.github/workflows/unit-tests.yml#L91

@xabbuh I see.

Btw, only 8.2 (high deps) is failing now. The rest is passing

[xabbuh]

I don't think we need the bump here. Let's rather skip the added test when the new exception class does not exist. Otherwise we won't be able to test the Serializer component with the real lowest allowed release of the PropertyAccess component.

[fabpot]

Thank you @seferov.

[yareckon]

Question, @fabpot set the milestone here for both 6.3 and 7.1, but the merge only happened for 7.1. This inscrutability for the error messages still persists on 6.4.x -- the LTS branch. Could we get an official patch for that?

[OskarStark]

Even if it looks like a bug, I have doubts about whether this should go to 7.1 as an improvement.

I agree with this sentiment. Technically it's a new feature because it adds a new class so let's target 7.1.

This is a new feature and will not be backported to 6.4 and the milestone is set for 7.1 only.

[yareckon]

Thanks for the response even if it's not the one I hoped for.