Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PropertyAccessor::getReadAccessInfo() throws exception that could be more specific #46867

Closed
patrickmaynard opened this issue Jul 6, 2022 · 7 comments · May be fixed by symfony/form#32
Closed

Comments

@patrickmaynard
Copy link
Contributor

patrickmaynard commented Jul 6, 2022

Symfony version(s) affected

5,6, possibly others

Description

Currently, if a form field does not have an equivalent setter or adder and does not use the ['mapped' => false] option, we get a NoSuchPropertyException that complains about a lack of setters or adders when submitting the form.

(This is Symfony\Component\PropertyAccess\PropertyAccessor::getReadAccessInfo(), in the method's final else branch.)

That's fine, I guess, but it can be a little bit confusing. Would it be safe for me to open a pull request adding a reference to ['mapped' => false] as a possible fix in the text of the exception? Or is this method used in non-form contexts?

How to reproduce

Create a form field in a form type that doesn't have a setter or adder associated with it in the equivalent Doctrine entity, and don't pass any options to the field. Then submit the form. You should get a nice NoSuchPropertyException talking about the missing methods, but it won't mention anything about how to solve the issue if you don't want to add those methods.

Possible Solution

I'm thinking about opening a pull request to change this part of Symfony\Component\PropertyAccess\PropertyAccessor::getReadAccessInfo() ...

$access[self::ACCESS_NAME] = sprintf(
    'Neither the property "%s" nor one of the methods "%s()" '.
-    'exist and have public access in class "%s".',
+    'exist and have public access in class "%s".' Consider adding one of those methods, or if you don't want a direct mapping, use ['mapped' => false] as a form field option.', 
    $property,
    implode('()", "', $methods),
    $reflClass->name
);

... but I first need to know that it's safe for me to do so without accidentally adding misleading instructions about form options to contexts that don't involve forms.

Please comment!

Additional Context

No response

@patrickmaynard patrickmaynard changed the title PropertyAccessor::getReadAccessInfo() throws error that could be more specific PropertyAccessor::getReadAccessInfo() throws exception that could be more specific Jul 8, 2022
@xabbuh
Copy link
Member

xabbuh commented Jul 9, 2022

Adding this to the PropertyAccess component isn't something that we can do as the component can be and is used without Symfony forms. However, the place where we leverage the property accessor in the Form component is the PropertyPathAccessor. We could catch the exception there and tweak the exception message before rethrowing it.

@patrickmaynard
Copy link
Contributor Author

@xabbuh great! I'll try to open a pull request tonight modifying PropertyPathAccessor. I appreciate the feedback.

@patrickmaynard
Copy link
Contributor Author

No luck -- I had some issues getting PHP 8.1 on my mac last night, which prevented me from installing dependencies and running tests. I'll try again sometime in the coming weeks.

@OskarStark
Copy link
Contributor

If you like you can edit the files via Github UI and create a PR, tests are then executed automatically for you on CI

@OskarStark OskarStark changed the title PropertyAccessor::getReadAccessInfo() throws exception that could be more specific PropertyAccessor::getReadAccessInfo() throws exception that could be more specific Jul 14, 2022
patrickmaynard added a commit to patrickmaynard/form that referenced this issue Aug 25, 2022
Adds more helpful exceptions when properties cannot be accessed/set
using forms.
Closes symfony/symfony#46867.
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Friendly reminder that this issue exists. If I don't hear anything I'll close this.

@alexislefebvre
Copy link
Contributor

Dear bot, please don't close this issue yet.

@carsonbot carsonbot removed the Stalled label Jul 4, 2023
fabpot added a commit that referenced this issue Mar 17, 2024
…ns (patrickmaynard)

This PR was squashed before being merged into the 7.1 branch.

Discussion
----------

46867 - add more helpful property path accessor exceptions

| Q             | A
| ------------- | ---
| Branch? | 6.3
| Bug fix? | no
| New feature? | no
| Deprecations? | no
| Tickets | #46867
| License | MIT
| Doc PR | none

Dear reviewers,

This small modification adds more helpful exceptions when properties cannot be accessed/set
using forms. Only one file was modified, so it should be relatively easy to review. Please feel free to leave a comment if you have any questions about what I'm doing here, and thanks for your work as a reviewer on an open-source project!

All the best,

Patrick

Commits
-------

8d87a67 46867 - add more helpful property path accessor exceptions
symfony-splitter pushed a commit to symfony/form that referenced this issue Mar 17, 2024
…ns (patrickmaynard)

This PR was squashed before being merged into the 7.1 branch.

Discussion
----------

46867 - add more helpful property path accessor exceptions

| Q             | A
| ------------- | ---
| Branch? | 6.3
| Bug fix? | no
| New feature? | no
| Deprecations? | no
| Tickets | symfony/symfony#46867
| License | MIT
| Doc PR | none

Dear reviewers,

This small modification adds more helpful exceptions when properties cannot be accessed/set
using forms. Only one file was modified, so it should be relatively easy to review. Please feel free to leave a comment if you have any questions about what I'm doing here, and thanks for your work as a reviewer on an open-source project!

All the best,

Patrick

Commits
-------

8d87a672a2 46867 - add more helpful property path accessor exceptions
@fabpot fabpot closed this as completed Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants