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

Throw less misleading exception when property access not found #19141

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@bramtweedegolf
Contributor

bramtweedegolf commented Jun 22, 2016

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

This comment has been minimized.

Contributor

bramtweedegolf commented Jun 22, 2016

Sorry, status should not be bug but enhancement.

@stof

This comment has been minimized.

Member

stof commented Jun 22, 2016

this should be covered by a test

@fabpot

This comment has been minimized.

Member

fabpot commented Jun 23, 2016

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]) {

This comment has been minimized.

@phansys

phansys Jun 24, 2016

Contributor

An elseif expression after an uncaught exception looks weird. IMHO, it should be more semantic to replace it with a new if.

This comment has been minimized.

@phansys

phansys Aug 24, 2016

Contributor

@nicolas-grekas: Coding Standards > Structure

Do not use else, elseif, break after if and case conditions which return or throw something;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 24, 2016

Member

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.

This comment has been minimized.

@phansys

phansys Aug 24, 2016

Contributor

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 {
    // ...
}

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 24, 2016

Member

But this code doesn't do the same...

This comment has been minimized.

@phansys

phansys Aug 24, 2016

Contributor

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]))

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 24, 2016

Member

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...

This comment has been minimized.

@phansys

phansys Aug 24, 2016

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 24, 2016

Member

With your code, if !$access[self::ACCESS_HAS_PROPERTY] && property_exists($object, $property) is true, an exception will be thrown anyway.

This comment has been minimized.

@phansys

phansys Aug 24, 2016

Contributor

Very good, finally I get your point. Sorry for the misunderstanding and thank you for the time you took to explain this.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Aug 9, 2016

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Aug 24, 2016

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]) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Aug 24, 2016

Member

@bramtweedegolf can you please move this elseif one step down, to put all throwing cases at the end?

@fabpot

This comment has been minimized.

Member

fabpot commented Sep 14, 2016

@bramtweedegolf Can you take the latest comments into account and add some unit tests to finish this PR? Thanks.

@nicolas-grekas nicolas-grekas added this to the 3.1 milestone Dec 6, 2016

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 8, 2016

Thank you @bramtweedegolf.

nicolas-grekas added a commit that referenced this pull request Dec 8, 2016

bug #19141 Throw less misleading exception when property access not f…
…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

This was referenced Dec 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment