Skip to content

#71 Null value matching problem #75

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Mar 17, 2016

Conversation

partikus
Copy link
Contributor

@norzechowicz code review please

@partikus partikus force-pushed the bugfix/nullable-value-matching branch from 0a117a1 to 9518da0 Compare March 15, 2016 08:05
@partikus
Copy link
Contributor Author

@norzechowicz if we want to fix nullable value then we cannot rely on getValue() !== null,
In order to keep backward compatibility and support for PropertyAccessor v2.3 I'd like to suggest write custom property accessor with isReadable method or since version 2.0 we can switch PA to newer version >=2.5

What do you think? Have you got any advices?

@@ -203,6 +203,7 @@ private function getPropertyAccessor()
}

$accessorBuilder = PropertyAccess::createPropertyAccessorBuilder();
$accessorBuilder->enableExceptionOnInvalidIndex();
Copy link
Member

Choose a reason for hiding this comment

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

why u enabled that option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if exceptionOnInvalidIndex is disabled then isReadable method returns true for unexisting properties of array, but if this option is enabled isReadable method catches all exceptions (eg InvalidIndex) and returns false. That's why I enabled exceptions.
Unfortunately this method was added in 2.5

e.g.
https://github.com/symfony/property-access/blob/2.5/PropertyAccessor.php#L134

Copy link
Member

Choose a reason for hiding this comment

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

Well in that case I guess you can just update dependency to 2.5^ in this PR (it will be tagged as 2.0 anyway). Recommended solution for versions 1.0 and 1.1 will be update to 2.0 for now, and maybe somebody will find a way to fix it also there.

@partikus
Copy link
Contributor Author

I've prepared compatible version with 2.3^

@norzechowicz cr ping

@norberttech
Copy link
Member

wow cool! Could you please squash those commits into one? I would need to cherry pick them to version 1.1 and 1.0 also probably.

@partikus partikus force-pushed the bugfix/nullable-value-matching branch from 25d7696 to d7fdd51 Compare March 15, 2016 11:49
@partikus
Copy link
Contributor Author

@norzechowicz done

@norberttech
Copy link
Member

@partikus thanks! I will merge it later, after that I will merge OR operator and then I will release rc2. 👍

@partikus
Copy link
Contributor Author

@norzechowicz good news, thanks!

norberttech pushed a commit that referenced this pull request Mar 17, 2016
@norberttech norberttech merged commit 87ff316 into coduo:master Mar 17, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants