Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Throw less misleading exception when property access not found#19141
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
bramtweedegolf commentedJun 22, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Sorry, status should not be bug but enhancement. |
stof commentedJun 22, 2016
this should be covered by a test |
fabpot commentedJun 23, 2016
I think that this qualifies as a bug fix. Can you add a test for that case? |
| $object->$property =$value; | ||
| }elseif (self::ACCESS_TYPE_NOT_FOUND ===$access[self::ACCESS_TYPE]) { | ||
| thrownewNoSuchPropertyException(sprintf('Could not determine access type for property "%s".',$property)); | ||
| }elseif (self::ACCESS_TYPE_MAGIC ===$access[self::ACCESS_TYPE]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Anelseif expression after an uncaught exception looks weird. IMHO, it should be more semantic to replace it with a newif.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas:Coding Standards > Structure
Do not use else, elseif, break after if and case conditions which return or throw something;
nicolas-grekasAug 24, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't know what you have in mind but if you mean just turning this elseif into an if, then the code won't mean the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I mean a better form could be:
// ...}elseif (self::ACCESS_TYPE_NOT_FOUND ===$access[self::ACCESS_TYPE]) {thrownewNoSuchPropertyException(sprintf('Could not determine access type for property "%s".',$property));}if (self::ACCESS_TYPE_MAGIC ===$access[self::ACCESS_TYPE]) {$object->{$access[self::ACCESS_NAME]}($value);}else {// ...}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
But this code doesn't do the same...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That would't be changed, I mean onlyelse/elseif blocks directly adjacent afterreturn orthrow statments (in this case,if (self::ACCESS_TYPE_MAGIC === $access[self::ACCESS_TYPE]))
nicolas-grekasAug 24, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm sorry but this is not the same... all conditions have to be checked before comming to that line, which means all previous conditions before have an impact on the conditions below. If you break the chain, you break the code...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The conditions after anif that evaluates totrue aren't checked (else /elseif) and the script flow stops when an uncaught exception is found, so which is the sense on having anelseif after a broken execution? IMO, this must be anif. I'm sharing a gist with the whole method just in order to clarify my thoughts:https://gist.github.com/phansys/42deea38ddcbc1d14b82654d92e8d7e0#file-propertyaccessor-php-L25
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
With your code, if!$access[self::ACCESS_HAS_PROPERTY] && property_exists($object, $property) is true, an exception will be thrown anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Very good, finally I get your point. Sorry for the misunderstanding and thank you for the time you took to explain this.
nicolas-grekas commentedAug 9, 2016
ping@bramtweedegolf |
nicolas-grekas commentedAug 24, 2016
Status: needs work |
| // fatal error. | ||
| $object->$property =$value; | ||
| }elseif (self::ACCESS_TYPE_NOT_FOUND ===$access[self::ACCESS_TYPE]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@bramtweedegolf can you please move this elseif one step down, to put all throwing cases at the end?
fabpot commentedSep 14, 2016
@bramtweedegolf Can you take the latest comments into account and add some unit tests to finish this PR? Thanks. |
nicolas-grekas commentedDec 8, 2016
Thank you@bramtweedegolf. |
…ound (bramtweedegolf)This PR was submitted for the master branch but it was merged into the 3.1 branch instead (closes#19141).Discussion----------Throw less misleading exception when property access not foundPrevent throwing a NoSuchPropertyException with a somewhat misleading message "Neither the property "X" nor one of the methods "addX()"/"removeX()", "setX()", "x()", "__set()" or "__call()" exist and have public access in class when the access cannot be determined, for instance if the doctrine schema is not up to date.| Q | A || --- | --- || Branch? | 3.1 for fixes || Bug fix? | no || New feature? | no || BC breaks? | no || Deprecations? | no || Tests pass? | yes || License | MIT |Commits-------ec28da4 Throw less misleading exception when property access not found
Uh oh!
There was an error while loading.Please reload this page.
Prevent throwing a NoSuchPropertyException with a somewhat misleading message "Neither the property "X" nor one of the methods "addX()"/"removeX()", "setX()", "x()", "__set()" or "__call()" exist and have public access in class when the access cannot be determined, for instance if the doctrine schema is not up to date.