Skip to content

Implement ParameterAttributeCollector #33

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jun 3, 2025

implement the necessary plumbing which allows us to find targets for attributes on method parameters

ports the implementation from ondrejmirtes#2 into this upstream repo

@clxmstaab clxmstaab force-pushed the method-params branch 2 times, most recently from 6174bc8 to 342e40c Compare June 3, 2025 10:27
@staabm staabm marked this pull request as ready for review June 3, 2025 10:30
@olvlvl
Copy link
Owner

olvlvl commented Jun 11, 2025

Hi @staabm, thank you for your contribution. What would the use case for this feature be?

@staabm
Copy link
Contributor Author

staabm commented Jun 11, 2025

Hey,

we use this implementation to make the lib work for PHPStan DI-Container configuration.
In this particular case we use it to wire parameters based on attributes.

see e.g. https://github.com/phpstan/phpstan-src/pull/4042/files#diff-3f4dd13f82480303d89c9bf92a5f81a457113ac25e44f7604f89503f210a18b4L27

@olvlvl olvlvl added this to the v2.1.0 milestone Jun 11, 2025
@olvlvl olvlvl self-requested a review June 11, 2025 09:32
@olvlvl olvlvl added the enhancement New feature or request label Jun 11, 2025
Copy link
Owner

@olvlvl olvlvl left a comment

Choose a reason for hiding this comment

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

I'm considering the naming "methodAttributes". The class is ReflectionAttribute, I think we should match the naming. Would it clash with other parameter types?

Also, please rebase you're branch. I've made some changes to decouple the collector from IOInterface.

/**
* @param array<TransientTargetMethod> $methodAttributes
* @param array<array<TransientTargetMethodParameter>> $methodParameterAttributes
* @return void
Copy link
Owner

Choose a reason for hiding this comment

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

Let's remove this one, it's already in the signature.

* @param array<array<TransientTargetMethodParameter>> $methodParameterAttributes
* @return void
*/
private function collectMethodAndParameterAttributes(string $class, \ReflectionMethod $methodReflection, array &$methodAttributes, array &$methodParameterAttributes): void
Copy link
Owner

Choose a reason for hiding this comment

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

Let's split this over multiple lines.

Suggested change
private function collectMethodAndParameterAttributes(string $class, \ReflectionMethod $methodReflection, array &$methodAttributes, array &$methodParameterAttributes): void
private function collectMethodAndParameterAttributes(
string $class,
\ReflectionMethod $methodReflection,
array &$methodAttributes,
array &$methodParameterAttributes
): void {

Copy link
Owner

Choose a reason for hiding this comment

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

Also, I'd like it better if we were not using references… but I can live with it.

}

$parameterAttributes = $parameterAttributeCollector->collectAttributes($methodReflection);
if ($parameterAttributes !== []) {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
if ($parameterAttributes !== []) {
if ($parameterAttributes) {

Simpler to read.

] = $classAttributeCollector->collectAttributes($class);
} catch (Throwable $e) {
$this->io->error(
"Attribute collection failed for $class: {$e->getMessage()}",
);
}

$this->state[$class] = [ time(), $classAttributes, $methodAttributes, $propertyAttributes ];
$this->state[$class] = [ time(), $classAttributes, $methodAttributes, $propertyAttributes, $methodParameterAttributes ];
Copy link
Owner

Choose a reason for hiding this comment

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

Let's split this one.

Suggested change
$this->state[$class] = [ time(), $classAttributes, $methodAttributes, $propertyAttributes, $methodParameterAttributes ];
$this->state[$class] = [
time(),
$classAttributes,
$methodAttributes,
$propertyAttributes,
$methodParameterAttributes,
];

Comment on lines +6 to +10
use PhpParser\Node;
use PhpParser\Node\Stmt\ClassLike;
use PhpParser\NodeTraverser;
use PhpParser\NodeVisitorAbstract;
use PhpParser\Parser;
Copy link
Owner

Choose a reason for hiding this comment

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

Unused imports.

Suggested change
use PhpParser\Node;
use PhpParser\Node\Stmt\ClassLike;
use PhpParser\NodeTraverser;
use PhpParser\NodeVisitorAbstract;
use PhpParser\Parser;

{
$funcParameterAttributes = [];
foreach ($reflectionFunctionAbstract->getParameters() as $parameter) {
$attributes = $parameter->getAttributes();
Copy link
Owner

Choose a reason for hiding this comment

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

We initialize the variable here, but it's used much later. Besides, the variable is only used once, we could do without.

Suggested change
$attributes = $parameter->getAttributes();

$paramLabel = $functionName . '(' . $parameterName . ')';
}

foreach ($attributes as $attribute) {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
foreach ($attributes as $attribute) {
foreach ($parameter->getAttributes() as $attribute) {

Comment on lines +9 to +15
{
public string $label;
public function __construct(
string $label = ''
) {
$this->label = $label;
}
Copy link
Owner

Choose a reason for hiding this comment

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

We can use promoted properties here:

Suggested change
{
public string $label;
public function __construct(
string $label = ''
) {
$this->label = $label;
}
public function __construct(
public string $label = ''
) {
}

Comment on lines +10 to +18
public string $label;
public string $moreData;
public function __construct(
string $label = '',
string $moreData = ''
) {
$this->label = $label;
$this->moreData = $moreData;
}
Copy link
Owner

Choose a reason for hiding this comment

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

We can use promoted properties here:

Suggested change
public string $label;
public string $moreData;
public function __construct(
string $label = '',
string $moreData = ''
) {
$this->label = $label;
$this->moreData = $moreData;
}
public function __construct(
public string $label = '',
public string $moreData = ''
) {
}

use Acme81\Attribute\ParameterA;

function aFunc(
#[ParameterA("my function parameter label")]
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should make it more apparent in the tests that we don't want this attribute to be collected; for example, with a $this->assertNotExists().

@ondrejmirtes
Copy link

By the way, I just wanted to show my appreciation for this package: it's awesome! It enabled me to use attributes in PHPStan to trim down the config files. I plan to publish a community update about this, but here's already an issue with some description and links to diffs what it allowed me to do: nette/di#324

I can also keep PHP 7.4 support because the attributes file is baked into the PHAR, so runtime inspection is not needed :)

@olvlvl
Copy link
Owner

olvlvl commented Jun 16, 2025

I opened another PR to complete the changes. I tested the branch on a few projects with success.

@staabm @ondrejmirtes Could you test the changes with your project? Careful, I renamed a few things. You'll have to use the findTargetParameters() method now.

#37

@staabm
Copy link
Contributor Author

staabm commented Jun 17, 2025

implemented via #37

@staabm staabm closed this Jun 17, 2025
@staabm staabm deleted the method-params branch June 17, 2025 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants