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.

Sign up for GitHub

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

Jump to bottom

Allowing non-nullable properties for AccessInterceptorScopeLocalizer #683

Open
petr-buchin opened this issue Apr 7, 2021 · 6 comments · May be fixed by #685
Open

Allowing non-nullable properties for AccessInterceptorScopeLocalizer #683

petr-buchin opened this issue Apr 7, 2021 · 6 comments · May be fixed by #685

Comments

@petr-buchin
Copy link

petr-buchin commented Apr 7, 2021 •
edited
Loading

This is a follow up on #682.

After doing some research, I realized that enabling AccessInterceptorScopeLocalizer to work with non-nullable properties is pretty straightforward.

For now checking for non-accessible properties looks like follows in class Properties:

return new self(array_filter($this->properties, static function (ReflectionProperty $property): bool {
    if (! $property->hasType()) {
        return false;
    }

    return ! array_key_exists(
        $property->getName(),
        // https://bugs.php.net/bug.php?id=77673
        array_flip(array_keys($property->getDeclaringClass()->getDefaultProperties()))
    );
}));

To fix this behavior and allow non-nullable properties on objects being proxied, this code can be changed to something like follows:

return new self(array_filter($this->properties, static function (ReflectionProperty $property) use($realObject): bool {
    if (! $property->hasType()) {
        return false;
    }
    
    if ($property->getType()->allowsNull() && $property->getType()->hasDefaultValue()) {
        return false;
    }

    return ! $property->isInitialized($realObject);
}));

This code allows to proxy objects with non-nullable properties and moves responsibility of initializing properties to the user of this library.

However, the problem is that classes Properties and BindProxyProperties does not have access to the object being proxied, thus cannot use method ReflectionProperty::isInitialized().

It's possible to pass the instance of an object being proxied to this classes, but that would require changing signatures of some interface methods, and that would be a BC break.

I was thinking about using a temporary workaround (before next major release), such as ProxyContext class with static property, like ProxyContext::$objectBeingProxied, that would allow classes like BindProxyProperties to have access to the object being proxied without changing interfaces, but that is admittedly clunky.

@Ocramius do you have any thoughts on this?
@Ocramius
Copy link
Owner

Ocramius commented Apr 7, 2021

@petr-buchin Properties is a build-time construct, therefore use ($realObject) cannot work there in first place.

A runtime reflection call at instantiation, if needed, needs to be baked into the generated code.

@petr-buchin
Copy link
Author

@Ocramius thanks for quick answer.

Probably this logic may be baked into the generated code, if needed.

But what do you think about the proposed solution?

If you agree with the solution itself, I could try to implement it.

@Ocramius
Copy link
Owner

Ocramius commented Apr 7, 2021

But what do you think about the proposed solution?

Adding a runtime object to the Properties signature is a no-go, as explained: that's a build-time component of the \Generator\ sub-namespace, and should not be used at runtime.

Adding code to the generated named constructors is feasible, but we need an extensive integration test verifying that it works as expected.

@petr-buchin
Copy link
Author

Adding code to the generated named constructors is feasible, but we need an extensive integration test verifying that it works as expected.

@Ocramius thanks, I'll try to implement it along with tests and create a PR

@Ocramius
Copy link
Owner

Ocramius commented Apr 7, 2021

@petr-buchin please do start from integration test and build a PoC starting from there, even if quick & dirty.

Do not start from the sources, but really do start from the test suite, by adding a class that looks like what you are trying to proxy, and making it work from there (even by having other tests failing, as a start).

@petr-buchin
Copy link
Author

@Ocramius no problems, I'll go with TDD here, thanks!

@petr-buchin petr-buchin linked a pull request Apr 12, 2021 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support for non-nullable typed properties petr-buchin/ProxyManager
2 participants
@Ocramius @petr-buchin