Skip to content

[Validator] Add Xml constraint #57365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
Open

[Validator] Add Xml constraint #57365

wants to merge 1 commit into from
+321 −0

Conversation

sfmok
Copy link
Contributor

@sfmok sfmok commented Jun 10, 2024 •
edited by OskarStark
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues No
License MIT

Add Xml constraint

@carsonbot carsonbot added this to the 7.2 milestone Jun 10, 2024
@carsonbot carsonbot changed the title Add YAML, XML and JWT format constraints [Validator] Add YAML, XML and JWT format constraints Jun 12, 2024
@OskarStark
Copy link
Contributor

Did you check

?

@OskarStark
Copy link
Contributor

OskarStark commented Jun 15, 2024 •
edited
Loading

Yaml constraint was merged, so please rebase

@OskarStark OskarStark changed the title [Validator] Add YAML, XML and JWT format constraints [Validator] Add XML and JWT format constraints Jun 15, 2024
@sfmok sfmok force-pushed the feat/new-constraints branch from 9708e02 to 1edfe83 Compare June 17, 2024 13:47
@sfmok sfmok changed the title [Validator] Add XML and JWT format constraints [Validator] Add XML constraint Jun 17, 2024
@sfmok
Copy link
Contributor Author

sfmok commented Jun 17, 2024

I've made the following changes:

  • Removed format abstraction
  • Removed the Yaml constraint from this PR since it has already been merged
  • Reverted the Json constraint changes to prevent breaking the translation
  • Remove the Jwt constraint for now, as verifying the signature requires extensive work. I will likely push a separate PR for it later.
  • Adjusted the Xml constraint to support only the modern method of defining constraints

@OskarStark OskarStark changed the title [Validator] Add XML constraint [Validator] Add Xml constraint Jun 17, 2024
OskarStark
OskarStark approved these changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Needs a rebase

@OskarStark OskarStark requested a review from stof June 17, 2024 20:39
@sfmok sfmok force-pushed the feat/new-constraints branch from 4716da3 to ebdab1f Compare June 17, 2024 21:35
alexandre-daubois
alexandre-daubois reviewed Jun 18, 2024
View reviewed changes
Copy link
Member

@alexandre-daubois alexandre-daubois left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a few nitpicks 🙂 Nice PR!

@sfmok sfmok force-pushed the feat/new-constraints branch from 539154e to ea4c209 Compare June 20, 2024 16:27
nicolas-grekas
nicolas-grekas reviewed Jun 24, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Asserting that a file is XML is pretty weak as a check: one always expects some structure in the XML. It'd be natural to me to allow passing an XSD, isn't it? Up to improve the PR in this direction?

@sfmok sfmok force-pushed the feat/new-constraints branch 2 times, most recently from b7ae934 to 08515a5 Compare June 25, 2024 14:02
@sfmok
Copy link
Contributor Author

sfmok commented Jun 27, 2024

Asserting that a file is XML is pretty weak as a check: one always expectes some structure in the XML. It'd be natural to me to allow passing an XSD, isn't it? Up to improve the PR in this direction?

@nicolas-grekas Thank you for your feedback. The constraint is intended to assert a string value rather than a file.
I'm not sure about allowing XSD checks, but I will consider supporting it if we receive more positive feedback either before or after merging the PR

94noni
94noni approved these changes Aug 1, 2024
View reviewed changes
Copy link
Contributor

@94noni 94noni left a comment

Choose a reason for hiding this comment

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

Nice

@sfmok
Copy link
Contributor Author

sfmok commented Nov 5, 2024

Any updates?

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@fabpot
Copy link
Member

fabpot commented Feb 26, 2025

Asserting that a file is XML is pretty weak as a check: one always expectes some structure in the XML. It'd be natural to me to allow passing an XSD, isn't it? Up to improve the PR in this direction?

@nicolas-grekas Thank you for your feedback. The constraint is intended to assert a string value rather than a file. I'm not sure about allowing XSD checks, but I will consider supporting it if we receive more positive feedback either before or after merging the PR

I also think that supporting validating the XML content against a schema would make this feature more useful.

nicolas-grekas
nicolas-grekas requested changes Mar 28, 2025
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Status: needs work

Still up to continue the PR?

@alexandre-daubois alexandre-daubois self-requested a review March 28, 2025 09:38
@sfmok
Copy link
Contributor Author

sfmok commented Mar 28, 2025

@nicolas-grekas Yes, I'll work on it when I have some free time.

@sfmok sfmok force-pushed the feat/new-constraints branch from 92712d9 to 0a18fe2 Compare April 17, 2025 19:45
@sfmok
Copy link
Contributor Author

sfmok commented Apr 18, 2025

@nicolas-grekas & @fabpot: Thanks again for the feedback. XSD schema validation is now supported.

alexandre-daubois
alexandre-daubois reviewed Apr 18, 2025
View reviewed changes
src/Symfony/Component/Validator/Constraints/Xml.php Outdated
) {
parent::__construct(null, $groups, $payload);

if (null !== $this->schemaPath && !file_exists($this->schemaPath)) {
Copy link
Member

@alexandre-daubois alexandre-daubois Apr 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Let's use is_readable() to ensure that the file exists and can actually be read:

Suggested change
if (null !== $this->schemaPath && !file_exists($this->schemaPath)) {
if (null !== $this->schemaPath && !is_readable($this->schemaPath)) {

Copy link
Member

Choose a reason for hiding this comment

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

In this case, you also need to change the error message: "file does not exist or cannot be read".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your valuable feedback. Resolved

@sfmok sfmok force-pushed the feat/new-constraints branch from 0a18fe2 to 5241b22 Compare April 18, 2025 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GromNaN GromNaN GromNaN left review comments

@OskarStark OskarStark OskarStark approved these changes

@alexandre-daubois alexandre-daubois alexandre-daubois approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas requested changes

@94noni 94noni 94noni approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Feature Status: Needs Work Validator
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.
9 participants
@sfmok @OskarStark @fabpot @nicolas-grekas @GromNaN @stof @94noni @alexandre-daubois @carsonbot