Skip to content

Bump to phpdoc-parser 1.23 and fill construct() attributes#4590

Merged
samsonasik merged 7 commits intomainfrom
bump-phpdoc-parser-2
Jul 24, 2023
Merged

Bump to phpdoc-parser 1.23 and fill construct() attributes#4590
samsonasik merged 7 commits intomainfrom
bump-phpdoc-parser-2

Conversation

@samsonasik
Copy link
Copy Markdown
Member

No description provided.

@samsonasik samsonasik requested a review from TomasVotruba as a code owner July 24, 2023 12:52
Comment on lines +52 to +53
// usedAttributes
['lines' => true, 'indexes' => true],
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is based on @ondrejmirtes suggestion at rectorphp/rector#7959 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make sure to pass the same array also to other classes that use it - ConstExprParser, TypeParser, PhpDocParser.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thank you @ondrejmirtes, I will check 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ondrejmirtes I updated TypeParser and ConstExprParser usedAttributes 41a727a

Comment on lines +56 to +60
// textBetweenTagsBelongsToDescription, default to false, exists since 1.23.0
// should be can used for
// @param int $a
// paramA description
true
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this exist since phpdoc-parser 1.23.0 that may be used for drupal use case

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@TomasVotruba it seems working to preserve next line text part of @param d401cc6 👍

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ondrejmirtes the spaced new line empty seems cause indentation gone, given the following code:

class Foo extends FooBar
{
    /**
     * @param \DateTime $foo
     *
     * Storage here is Foo\Storage. Not \Storage.
     * @return Storage
     */
    public function bar($foo){}
}

it goes:

  class Foo extends FooBar
 {
     /**
-     * @param DateTime $foo
-     *
-     * Storage here is Foo\Storage. Not \Storage.
-     * @return Storage
-     */
+     * @param DateTime $foo 
+    
+    Storage here is Foo\Storage. Not \Storage.
+    * @return Storage
+    */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@samsonasik Please report this properly in phpstan/phpdoc-parser, ideally with a failing test. I can't debug this through rector-src.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@ondrejmirtes thank you, I will try when I have a chance to reproduce a failing test case on phpstan/phpdoc-parser, I will set to default false for now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@TomasVotruba I reverted to false for now :), marked as @todo until it has solution for above empty line use case 2270001

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

@samsonasik
Copy link
Copy Markdown
Member Author

@TomasVotruba let's merge it ;)

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants