Skip to content

Conversation

staabm
Copy link

@staabm staabm commented May 28, 2025

opening a first draft so we can discuss directions

/**
* @return array<TransientTargetMethodParameter>
*/
public function collectAttributes(\ReflectionFunctionAbstract $reflectionFunctionAbstract): array
Copy link
Author

Choose a reason for hiding this comment

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

I have build this collector on ReflectionFunctionAbstract, so we can easily add support for function parameters

public const CACHE_DIR = '.composer-attribute-collector';
public const VERSION_MAJOR = 2;
public const VERSION_MINOR = 0;
public const VERSION_MINOR = 1;
Copy link
Author

@staabm staabm May 28, 2025

Choose a reason for hiding this comment

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

use new version to make sure a fresh attributes.php is generated and we don't need to invalide existing caches in tests.

attributes.php in consuming projects currently don't have a cache key or similar

Comment on lines +28 to +35
// classes
$targetClassesCode,
// methods
$targetMethodsCode,
// properties
$targetPropertiesCode,
// method parameters
$targetMethodParametersCode,
Copy link
Author

Choose a reason for hiding this comment

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

added comments to ease reading of the attributes.php

@coveralls
Copy link

coveralls commented May 28, 2025

Pull Request Test Coverage Report for Build 15401669993

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 91 of 156 (58.33%) changed or added relevant lines in 11 files are covered.
  • 43 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-1.2%) to 72.196%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ClassAttributeCollector.php 19 20 95.0%
src/Collection.php 19 25 76.0%
src/CachedParser.php 1 9 11.11%
src/AttributeGroupsReflector.php 0 19 0.0%
src/ParameterAttributeCollector.php 21 52 40.38%
Files with Coverage Reduction New Missed Lines %
src/ClassAttributeCollector.php 43 52.71%
Totals Coverage Status
Change from base Build 15277942570: -1.2%
Covered Lines: 457
Relevant Lines: 633

💛 - Coveralls

Copy link
Owner

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

This looks great! Looking forward to see this finished.

The PHP-Parser implementation will be verified by having tests succeed on PHP 7.4. Just beware - because of new $attribute(...$args), it cannot work with named arguments, so you can test it only with positional arguments like #[Foo(1, 2, 3)].

In phpstan-src this problem is avoided by downgrading named arguments to positional arguments even inside attributes.

@staabm
Copy link
Author

staabm commented Jun 2, 2025

while working on the php-parser based parts, I realized that atm this packages relies on runtime reflection, which means I cannot reflect on a class which uses hooked properties when on PHP 8.3 or lower.

do you agree that we need ondrejmirtes/better-reflection as a dependency to make it work with static reflection instead?
or do you see a different way forward? or should we support hooked properties only when PHP runtime is 8.4+?

@ondrejmirtes
Copy link
Owner

do you agree that we need ondrejmirtes/better-reflection

No, I don't want to complicate this with more dependencies.

We don't need support for property hooks now, you can omit it.

cannot be emulated via runtime reflection on PHP < 8.4
@staabm staabm marked this pull request as ready for review June 2, 2025 20:06
@staabm
Copy link
Author

staabm commented Jun 2, 2025

do you have a cs-fixer arround to fix the build? otherwise I think its good to go

@ondrejmirtes
Copy link
Owner

Try running vendor/bin/phpcbf

@staabm
Copy link
Author

staabm commented Jun 3, 2025

here we go

@ondrejmirtes ondrejmirtes merged commit bc4ca66 into ondrejmirtes:main Jun 3, 2025
8 checks passed
@ondrejmirtes
Copy link
Owner

Thank you!

@staabm staabm deleted the params branch June 3, 2025 09:40
@ondrejmirtes
Copy link
Owner

This is what your work allows us to do now: phpstan/phpstan-src#4042

@staabm
Copy link
Author

staabm commented Jun 3, 2025

nice. while porting this PR to the upstream, I found a bug. will fix in a moment

@staabm
Copy link
Author

staabm commented Jun 3, 2025

strike that.. it was a upstream only bug because of bad merge conflicts

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants