NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[PropertyInfo] Get short description from promoted properties in`PhpDocExtractor`#59540

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

Jump to bottom

Open

wuchen90 wants to merge1 commit intosymfony:7.4

base:7.4

Choose a base branch

fromwuchen90:fix-phpdoc-extractor

Open

[PropertyInfo] Get short description from promoted properties in`PhpDocExtractor`#59540

wuchen90 wants to merge1 commit intosymfony:7.4fromwuchen90:fix-phpdoc-extractor

Conversation

Copy link

Contributor

wuchen90 commentedJan 17, 2025•
edited
Loading

Q	A
Branch?	6.4
Bug fix?	yes
License	MIT

Currently this kind of phpdoc doesn't work to get description:

class Foo {/**   * @param mixed $foo This description can't be get by PhpDocExtractor   */publicfunction__construct(public$foo,  ) {}}

Whereas this one works:

class Foo {publicfunction__construct(/** @var mixed $foo This description can be get by PhpDocExtractor */public$foo,  ) {}}

wuchen90 requested a review fromdunglas as acode owner

January 17, 2025 16:45

carsonbot added Status: Needs Review Bug PropertyInfo labels

Jan 17, 2025

carsonbot added this to the6.4 milestone

Jan 17, 2025

Copy link

Contributor

OskarStark commentedJan 17, 2025

This is a feature and should target 7.3

OskarStark modified the milestones:6.4,7.3

Jan 17, 2025

Copy link

ContributorAuthor

wuchen90 commentedJan 17, 2025•
edited
Loading

@OskarStark

This is a feature and should target 7.3

I don't know if this is really a feature because it works well with@var annotation.
Only, when we document a method, we usually use@param and not@var.

Beside, this code below points out that we support promoted properties:

symfony/src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php

Line 239 in02db9c2

case$reflectionProperty?->isPromoted() &&$docBlock =$this->getDocBlockFromConstructor($class,$property):

mtarld requested changes

Jan 17, 2025

View reviewed changes

Copy link

Contributor

mtarld left a comment

Choose a reason for hiding this comment

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

Can you update the changelog as well, as it's a feature?

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.phpShow resolvedHide resolved

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php


		$paramDescription = $param->getDescription()?->render();

		if (null !== $paramDescription && '' !== $paramDescription) {

Copy link

Contributor

Choose a reason for hiding this comment

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

Suggested change

	if (null !==$paramDescription &&'' !==$paramDescription) {
	if ($paramDescription) {

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Are you sure?

<?php$paramDescription ='0';echo$paramDescription ?'ok' :'ko';// 'ko'echonull !==$paramDescription &&'' !==$paramDescription ?'ok' :'ko';// 'ok'

Copy link

Member

Choose a reason for hiding this comment

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

not sure0 a useful description either

Copy link

ContributorAuthor

wuchen90Jan 31, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Trimming0 won't make it less predictable?
I mean in the way that, from the user point of view, we naturally expect that empty string or null can be skipped.
But other than these two cases, shouldn't we return what the user has input or is it what Symfony used to do for this kind of code?
If we skip0 then I should add a test for it.

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Jan 17, 2025

Copy link

Contributor

OskarStark commentedJan 17, 2025

Looking at your code change this seems like it wasn't implemented before, there is no wrong behavior, the feature is just not existent, so a feature to me.

Copy link

Member

stof commentedJan 17, 2025

I think the implementation would now read@param in docblock of any properties, not just for the case of reading the constructor docblock for a promoted property.

wuchen90 force-pushed thefix-phpdoc-extractor branch from8a99cd3 tof6c61c1Compare

January 20, 2025 08:25

wuchen90 requested review fromlyrixx,yceruto,chalasr,OskarStark,jderusse andxabbuh ascode owners

January 20, 2025 08:25

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Jan 20, 2025

wuchen90 changed the base branch from6.4 to7.3

January 20, 2025 08:25

wuchen90 commented

Jan 20, 2025

View reviewed changes

Copy link

ContributorAuthor

wuchen90 left a comment

Choose a reason for hiding this comment

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

Target branch changed to 7.3.

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.phpShow resolvedHide resolved

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php


		$paramDescription = $param->getDescription()?->render();

		if (null !== $paramDescription && '' !== $paramDescription) {

Copy link

ContributorAuthor

wuchen90Jan 20, 2025

Choose a reason for hiding this comment

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

Are you sure?

<?php$paramDescription ='0';echo$paramDescription ?'ok' :'ko';// 'ko'echonull !==$paramDescription &&'' !==$paramDescription ?'ok' :'ko';// 'ok'

Copy link

Contributor

mtarld commentedJan 20, 2025

Indeed, forgot about it, you can keepnull !== $paramDescription && '' !== $paramDescription then 🙂

nicolas-grekas reviewed

Jan 28, 2025

View reviewed changes

src/Symfony/Component/PropertyInfo/Tests/Fixtures/Extractor/PromotedPropertiesWithDocBlock.php OutdatedShow resolvedHide resolved

src/Symfony/Component/PropertyInfo/Tests/Extractor/PhpDocExtractorTest.php OutdatedShow resolvedHide resolved

src/Symfony/Component/PropertyInfo/Extractor/PhpDocExtractor.php


		$paramDescription = $param->getDescription()?->render();

		if (null !== $paramDescription && '' !== $paramDescription) {

Copy link

Member

nicolas-grekasJan 28, 2025

Choose a reason for hiding this comment

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

not sure0 a useful description either

[PropertyInfo] Get short description from promoted properties in PhpD…

9141b68

…ocExtractor

wuchen90 force-pushed thefix-phpdoc-extractor branch fromf6c61c1 to9141b68Compare

January 31, 2025 08:52

OskarStark changed the title~~[PropertyInfo] Get short description from promoted properties in PhpDocExtractor~~[PropertyInfo] Get short description from promoted properties inPhpDocExtractor

Feb 10, 2025

OskarStark mentioned this pull request

Feb 10, 2025

[StructuredOutput] Support description from@paramphp-llm/llm-chain#202

Closed