Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Validator] AddXml 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.

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

Already on GitHub?Sign in to your account

Open
sfmok wants to merge1 commit intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromsfmok:feat/new-constraints

Conversation

sfmok
Copy link
Contributor

@sfmoksfmok commentedJun 10, 2024•
edited by OskarStark
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
IssuesNo
LicenseMIT

AddXml constraint

chapterjason, ostrolucky, and xfifix reacted with thumbs up emoji
@carsonbotcarsonbot added this to the7.2 milestoneJun 10, 2024
@carsonbotcarsonbot changed the titleAdd YAML, XML and JWT format constraints[Validator] Add YAML, XML and JWT format constraintsJun 12, 2024
@OskarStark
Copy link
Contributor

Did you check

?

sfmok and symfonyaml reacted with thumbs up emoji

@OskarStark
Copy link
Contributor

OskarStark commentedJun 15, 2024•
edited
Loading

Yaml constraint was merged, so please rebase

sfmok reacted with thumbs up emoji

@OskarStarkOskarStark changed the title[Validator] Add YAML, XML and JWT format constraints[Validator] Add XML and JWT format constraintsJun 15, 2024
@sfmoksfmokforce-pushed thefeat/new-constraints branch from9708e02 to1edfe83CompareJune 17, 2024 13:47
@sfmoksfmok changed the title[Validator] Add XML and JWT format constraints[Validator] Add XML constraintJun 17, 2024
@sfmok
Copy link
ContributorAuthor

I've made the following changes:

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

@OskarStarkOskarStark changed the title[Validator] Add XML constraint[Validator] AddXml constraintJun 17, 2024
Copy link
Contributor

@OskarStarkOskarStark 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

sfmok reacted with thumbs up emoji
Copy link
Member

@alexandre-dauboisalexandre-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 reacted with thumbs up emoji
Copy link
Member

@nicolas-grekasnicolas-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 and xfifix reacted with thumbs up emoji
@sfmoksfmokforce-pushed thefeat/new-constraints branch 2 times, most recently fromb7ae934 to08515a5CompareJune 25, 2024 14:02
@sfmok
Copy link
ContributorAuthor

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

Copy link
Contributor

@94noni94noni 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
ContributorAuthor

Any updates?

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

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.

sfmok reacted with thumbs up emoji

Copy link
Member

@nicolas-grekasnicolas-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?

sfmok reacted with thumbs up emoji
@sfmok
Copy link
ContributorAuthor

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

@sfmoksfmokforce-pushed thefeat/new-constraints branch from92712d9 to0a18fe2CompareApril 17, 2025 19:45
@sfmok
Copy link
ContributorAuthor

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

@sfmoksfmokforce-pushed thefeat/new-constraints branch from0a18fe2 to5241b22CompareApril 18, 2025 17:30
@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@GromNaNGromNaNGromNaN left review comments

@OskarStarkOskarStarkOskarStark approved these changes

@alexandre-dauboisalexandre-dauboisalexandre-daubois approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas requested changes

@94noni94noni94noni approved these changes

@stofstofAwaiting requested review from stof

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

9 participants
@sfmok@OskarStark@fabpot@nicolas-grekas@GromNaN@stof@94noni@alexandre-daubois@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp