[PropertyAccess][PropertyInfo] customize behavior for property hooks on read and write #59246
Conversation
c4ef85d to
399efcf
Compare
| public bool $virtualSetHookOnly { set => $value; } | ||
| public bool $virtualHook { get => true; set => $value; } | ||
| public bool $virtualSetHookOnly { set (bool $value) { } } | ||
| public bool $virtualHook { get => true; set (bool $value) { } } |
There was a problem hiding this comment.
I updated this fixtures because PHP does not report these properties as Virtual, see reproducer
0900ef7 to
379e96c
Compare
|
Tests are failing on 8.2 / highest deps because of other components (Bridge\twig, Bundle\FrameworkBundle, Component\HttpKernel). It leaves us with psalm for which PHP 8.4 support can be tracked here: vimeo/psalm#11107. Not sure how to deal with it here. |
src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
Outdated
Show resolved
Hide resolved
a396856 to
73f2e3f
Compare
|
@mtarld Thanks for your feedback, I think everything is covered now :) |
src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
Outdated
Show resolved
Hide resolved
bd1f4a1 to
0c5bb13
Compare
src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php
Outdated
Show resolved
Hide resolved
0c5bb13 to
0eaa1d4
Compare
|
Thanks @mtarld for your previous review, sorry I took so long to make the changes. |
0eaa1d4 to
0d100a9
Compare
|
@OskarStark thanks for your feedback, I renamed the variables. |
|
Hi @mtarld do you think we might be good on this pull request ? Thanks ! 🙏 |
|
Would it be sufficient to split those PR into two separate feature PRs? |
src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/PropertyAccess/Tests/PropertyAccessorTest.php
Outdated
Show resolved
Hide resolved
| if ($hasProperty && (($r = $reflClass->getProperty($property))->getModifiers() & $this->propertyReflectionFlags)) { | ||
| return new PropertyReadInfo(PropertyReadInfo::TYPE_PROPERTY, $property, $this->getReadVisibilityForProperty($r), $r->isStatic(), true, $this->propertyHasHook($r, 'get'), $this->propertyIsVirtual($r)); | ||
| } | ||
|
|
||
| if ($hasProperty && (($r = $reflClass->getProperty($property))->getModifiers() & $this->propertyReflectionFlags)) { | ||
| return new PropertyReadInfo(PropertyReadInfo::TYPE_PROPERTY, $property, $this->getReadVisibilityForProperty($r), $r->isStatic(), true); | ||
| if ($allowMagicGet && $reflClass->hasMethod('__get') && (($r = $reflClass->getMethod('__get'))->getModifiers() & $this->methodReflectionFlags)) { | ||
| return new PropertyReadInfo(PropertyReadInfo::TYPE_PROPERTY, $property, PropertyReadInfo::VISIBILITY_PUBLIC, false, $r->returnsReference(), false, false); | ||
| } |
There was a problem hiding this comment.
Why these ifs has been inverted?
There was a problem hiding this comment.
See that comment #59246 (comment)
0d100a9 to
3990cad
Compare
|
@mtarld thanks for the feedback |
There was a problem hiding this comment.
I'm not sure I like this new feature of PropertyAccess. Until now, PropertyAccess never accesses the internal state of the object through Reflection (Reflection is used only for instrospection of the class structure). All reads and writes are done through the public API of the class until now.
| @@ -77,19 +82,24 @@ class PropertyAccessor implements PropertyAccessorInterface | ||
| * or self::DISALLOW_MAGIC_METHODS for none | ||
| * @param int $throw A bitwise combination of the THROW_* constants | ||
| * to specify when exceptions should be thrown | ||
| * @param int $bypassPropertyHooks A bitwise combination of the BYPASS_PROPERTY_HOOK_* constants |
There was a problem hiding this comment.
@param int-mask-of<self::BYPASS_PROPERTY_HOOK_*> $bypassPropertyHooks so that static analysis tool know about the precise type and can report invalid usages.
There was a problem hiding this comment.
thx, didn't know that syntax
| @@ -47,6 +47,10 @@ class PropertyAccessor implements PropertyAccessorInterface | ||
| /** @var int Allow magic __call methods */ | ||
| public const MAGIC_CALL = ReflectionExtractor::ALLOW_MAGIC_CALL; | ||
|
|
||
| public const BYPASS_PROPERTY_HOOK_NONE = 0; | ||
| public const BYPASS_PROPERTY_HOOK_READ = 1 << 1; | ||
| public const BYPASS_PROPERTY_HOOK_WRITE = 1 << 0; |
There was a problem hiding this comment.
constants for a bit mask are generally defined in increasing order (so that new cases are added at the end of the list). This reverse order is unusual to me
3990cad to
7963304
Compare
…ty hooks on read and write
7963304 to
e4901fb
Compare
|
@stof I thought it would be very relevant in this component as this follows new opportunities offered by the language, wdyt ? |
This pull requests adds 2 new (tiny) features:
Have a nice day!