Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[PropertyAccess][PropertyInfo] customize behavior for property hooks on read and write#59246

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
xavierleune wants to merge1 commit intosymfony:7.4
base:7.4
Choose a base branch
Loading
fromxavierleune:feature/property-hooks

Conversation

xavierleune
Copy link
Contributor

@xavierleunexavierleune commentedDec 18, 2024•
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
Issues¤
LicenseMIT

This pull requests adds 2 new (tiny) features:

  • PropertyInfo: expose data around property hooks for readInfo and writeInfo
  • PropertyAccess: allow the end user to customize behavior around property hooks, to bypass them if needed

Have a nice day!

@carsonbotcarsonbot added this to the7.3 milestoneDec 18, 2024
@carsonbotcarsonbot changed the title[PropertyInfo] [PropertyAccess] Feature: customize behavior for property hooks on read and write[PropertyAccess][PropertyInfo] Feature: customize behavior for property hooks on read and writeDec 18, 2024
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) { } }
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I updated this fixtures because PHP does not report these properties as Virtual, seereproducer

@xavierleunexavierleuneforce-pushed thefeature/property-hooks branch 2 times, most recently from0900ef7 to379e96cCompareDecember 18, 2024 10:38
@xavierleune
Copy link
ContributorAuthor

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.

@xavierleunexavierleuneforce-pushed thefeature/property-hooks branch 3 times, most recently froma396856 to73f2e3fCompareDecember 20, 2024 13:39
@xavierleune
Copy link
ContributorAuthor

@mtarld Thanks for your feedback, I think everything is covered now :)

@xavierleunexavierleuneforce-pushed thefeature/property-hooks branch 2 times, most recently frombd1f4a1 to0c5bb13CompareDecember 20, 2024 16:10
@xavierleune
Copy link
ContributorAuthor

Thanks@mtarld for your previous review, sorry I took so long to make the changes.

@OskarStarkOskarStark changed the title[PropertyAccess][PropertyInfo] Feature: customize behavior for property hooks on read and write[PropertyAccess][PropertyInfo] customize behavior for property hooks on read and writeMar 21, 2025
@xavierleune
Copy link
ContributorAuthor

@OskarStark thanks for your feedback, I renamed the variables.

@xavierleune
Copy link
ContributorAuthor

Hi@mtarld do you think we might be good on this pull request ? Thanks ! 🙏

@OskarStark
Copy link
Contributor

OskarStark commentedApr 1, 2025•
edited
Loading

Would it be sufficient to split those PR into two separate feature PRs?

Comment on lines +395 to 401
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why theseifs has been inverted?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

See that comment#59246 (comment)

@xavierleune
Copy link
ContributorAuthor

@mtarld thanks for the feedback

Copy link
Member

@stofstof left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

@param int-mask-of<self::BYPASS_PROPERTY_HOOK_*> $bypassPropertyHooks so that static analysis tool know about the precise type and can report invalid usages.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

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

xavierleune reacted with thumbs up emoji
@xavierleunexavierleuneforce-pushed thefeature/property-hooks branch from7963304 toe4901fbCompareMay 5, 2025 09:12
@xavierleune
Copy link
ContributorAuthor

@stof I thought it would be very relevant in this component as this follows new opportunities offered by the language, wdyt ?

@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark left review comments

@mtarldmtarldmtarld requested changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

6 participants
@xavierleune@OskarStark@stof@mtarld@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp