Skip to content

Conversation

@yoeunes
Copy link
Contributor

@yoeunes yoeunes commented Oct 29, 2025

Q A
Branch? 7.3
Bug fix? yes
New feature? no
Deprecations? no
Issues n/a
License MIT

The set_error_handler closure was calling the non-existent method getParser(), which would cause a fatal error if a deprecation were ever triggered during parsing.

The fix instantiates the Parser as a local variable and passes it to the closure, resolving the issue and ensuring the validator remains stateless.

@stof
Copy link
Member

stof commented Oct 29, 2025

could you add a test covering this behavior to avoid regressions ?

@yoeunes yoeunes force-pushed the 7.3 branch 2 times, most recently from 2da0088 to aa3be59 Compare October 29, 2025 03:09
@yoeunes
Copy link
Contributor Author

yoeunes commented Oct 29, 2025

Hi @stof, I've added a new test that simulates deprecations on various lines, which tests that they are all correctly caught and converted into a ParseException.

{
}

protected function getParser(): Parser
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of introducing a getParser protected method in the constraint validator just for testing purpose (but which will then have to be covered by BC as it is not even internal), I would prefer to trigger a deprecation during an actual parsing to allowing keeping the parser fully local to the validator.
The lazy way would be to rely on one of the deprecations of the Yaml parser itself. But this won't be future-proof as those deprecations will become errors in 8.0. However, I have an idea that should work (I haven't tested it to confirm it):

  1. Create a fixture object in the testsuite that triggers a deprecation in its __unserialize method
  2. Create a Yaml string using !php/object with this object
  3. Run the validator with a constraint providing the Yaml::PARSE_OBJECT flag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @stof, you were right about the BC break, so I've implemented the test using your __unserialize idea instead and it works perfectly

@yoeunes yoeunes force-pushed the 7.3 branch 4 times, most recently from cff236b to 462875a Compare October 29, 2025 12:23
@nicolas-grekas
Copy link
Member

Thank you @yoeunes.

@nicolas-grekas nicolas-grekas merged commit 070fb79 into symfony:7.3 Oct 29, 2025
10 of 11 checks passed
This was referenced Nov 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants