Throw less misleading exception when property access not found #19141
Conversation
|
Sorry, status should not be bug but enhancement. |
|
this should be covered by a test |
|
I think that this qualifies as a bug fix. Can you add a test for that case? |
| @@ -651,6 +651,8 @@ private function writeProperty($zval, $property, $value) | |||
| // fatal error. | |||
|
|
|||
| $object->$property = $value; | |||
| } elseif (self::ACCESS_TYPE_NOT_FOUND === $access[self::ACCESS_TYPE]) { | |||
| throw new NoSuchPropertyException(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.
An elseif expression after an uncaught exception looks weird. IMHO, it should be more semantic to replace it with a new if.
There was a problem hiding this comment.
@nicolas-grekas: Coding Standards > Structure
Do not use else, elseif, break after if and case conditions which return or throw something;
There was a problem hiding this comment.
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.
I mean a better form could be:
// ...
} elseif (self::ACCESS_TYPE_NOT_FOUND === $access[self::ACCESS_TYPE]) {
throw new NoSuchPropertyException(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.
But this code doesn't do the same...
There was a problem hiding this comment.
That would't be changed, I mean only else/elseif blocks directly adjacent after return or throw statments (in this case, if (self::ACCESS_TYPE_MAGIC === $access[self::ACCESS_TYPE]))
There was a problem hiding this comment.
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.
The conditions after an if that evaluates to true aren't checked (else / elseif) and the script flow stops when an uncaught exception is found, so which is the sense on having an elseif after a broken execution? IMO, this must be an if. 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.
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.
Very good, finally I get your point. Sorry for the misunderstanding and thank you for the time you took to explain this.
|
ping @bramtweedegolf |
|
Status: needs work |
| @@ -651,6 +651,8 @@ private function writeProperty($zval, $property, $value) | |||
| // fatal error. | |||
|
|
|||
| $object->$property = $value; | |||
| } elseif (self::ACCESS_TYPE_NOT_FOUND === $access[self::ACCESS_TYPE]) { | |||
There was a problem hiding this comment.
@bramtweedegolf can you please move this elseif one step down, to put all throwing cases at the end?
|
@bramtweedegolf Can you take the latest comments into account and add some unit tests to finish this PR? Thanks. |
|
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 found 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. | 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
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.