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] Avoid triggering the autoloader for user-input values#40506

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

Merged
nicolas-grekas merged 1 commit intosymfony:4.4fromSeldaek:patch-11
Mar 23, 2021

Conversation

@Seldaek
Copy link
Member

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets
LicenseMIT
Doc PR

Following-up tohttps://twitter.com/seldaek/status/1372450636361502721 - mostly to see if the build passes or if this breaks some undocumented/unclear-to-me assumptions.

Essentially using theValid constraint should only validate objects if they exist as objects. If a user sends a string and that gets assigned to a property,Valid should not attempt autoloading that user-given string.

As far as I can tell, this is used in two places:

Nyholm, theofidry, derrabus, and OskarStark reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas changed the titleAvoid triggering the autoloader for user-input values[Validator] Avoid triggering the autoloader for user-input valuesMar 18, 2021
{
try {
if (!\is_object($object)) {
thrownewNoSuchMetadataException(sprintf('Cannot create metadata for non-objects. Got: "%s".',\gettype($object)));
Copy link
Contributor

Choose a reason for hiding this comment

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

TBF, the validation layer should not throw here, but reject the payload with a validation error, but I suppose "one thing at a time"

privatefunctionvalidateObject($object,string$propertyPath,array$groups,int$traversalStrategy,ExecutionContextInterface$context)
{
try {
if (!\is_object($object)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This execution path is missing a test case.

Test case would probably:

  1. register an autoloader
  2. verify if said autoloader is triggered with malformed input
  3. unregister autoloader

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yeah sorry but that goes beyond the time I'm willing to invest in this :D If someone wants to add a test please be my guest.

Copy link
Contributor

@OcramiusOcramiusMar 18, 2021
edited
Loading

Choose a reason for hiding this comment

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

That's understandable, it's not like these kind of issues appeared/regressed because of a lack of testing or such:trollface:

EDIT: to be clear, I fully understand/agree that time constraints are what they are. I just wouldn't merge without proper testing, not asking for random contributors to put more effort in it. Somebody else will pick it up, perhaps me, if I get to it.

Seldaek reacted with confused emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

@param object $object, so this seems suspicious :)

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh too quick:

it's explicitly passing anything in there to get the proper exception

but still :)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes and no.. if you do expect an object and you have no type hint, validing that you did indeed get an object and throwing otherwise doesn't seem that unreasonable.

Copy link
Member

Choose a reason for hiding this comment

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

but as we explicitly use that private method with non-objects to get the error message it generates, I would change the phpdoc tomixed instead (we don't want that exception to become a TypeError due to adding a nativeobject typehint)

nicolas-grekas, ro0NL, and Ocramius reacted with thumbs up emoji
@carsonbotcarsonbot changed the title[Validator] Avoid triggering the autoloader for user-input valuesAvoid triggering the autoloader for user-input valuesMar 18, 2021
@carsonbotcarsonbot changed the titleAvoid triggering the autoloader for user-input values[Validator] Avoid triggering the autoloader for user-input valuesMar 18, 2021
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.

I moved the check outside of thevalidateObject() method, it doesn't need to be inside of it.

I'm now merging even if a test case could be added.

@Ocramius thanks for the review. We'd be happy to merge a PR of yours to improve this even further!

@nicolas-grekas
Copy link
Member

Thank you@Seldaek.

@nicolas-grekasnicolas-grekas merged commit689056e intosymfony:4.4Mar 23, 2021
This was referencedMar 29, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+2 more reviewers

@OcramiusOcramiusOcramius requested changes

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

7 participants

@Seldaek@nicolas-grekas@Ocramius@stof@ro0NL@derrabus@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp