Skip to content

feat: PhpdocTypesFixer - support multiline array shapes#8893

Merged
keradus merged 4 commits intoPHP-CS-Fixer:masterfrom
6b7562617765726c6f73:feat_PhpdocTypesFixer_multiline_array_shapes
Jul 29, 2025
Merged

feat: PhpdocTypesFixer - support multiline array shapes#8893
keradus merged 4 commits intoPHP-CS-Fixer:masterfrom
6b7562617765726c6f73:feat_PhpdocTypesFixer_multiline_array_shapes

Conversation

@kubawerlos
Copy link
Copy Markdown
Member

Closes #6078

@kubawerlos kubawerlos force-pushed the feat_PhpdocTypesFixer_multiline_array_shapes branch from 931421a to b30b6fc Compare July 26, 2025 09:49
@coveralls
Copy link
Copy Markdown

coveralls commented Jul 26, 2025

Coverage Status

coverage: 94.756% (-0.009%) from 94.765%
when pulling 868de74 on 6b7562617765726c6f73:feat_PhpdocTypesFixer_multiline_array_shapes
into 125449b on PHP-CS-Fixer:master.

(?<array_shape>
(?<array_shape_name>(?i)(?:array|list|object)(?-i))
(?<array_shape_start>\h*\{\h*)
(?<array_shape_start>\h*\{[\h\v*]*)
Copy link
Copy Markdown
Contributor

@mvorisek mvorisek Jul 28, 2025

Choose a reason for hiding this comment

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

I think the main regex for types should not support *. * is not part of phpdoc grammar.

If this approach is needed, keep the original regex as is and let's have a 2nd one with something like str_replace('\h', '[\h\v*]', ....

My preference would be to never mix this parsing together and use proper nesting:

  1. parse /** comments
  2. strip /**, * at following lines and */
  3. now fix the "parsed comment content"
  4. restore /**, ˙*˙...

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.

I think the main regex for types should not support *. * is not part of phpdoc grammar.

How * is not part of phpdoc grammar?

Copy link
Copy Markdown
Contributor

@mvorisek mvorisek Jul 29, 2025

Choose a reason for hiding this comment

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

I mean part of the phpdoc type. IMO you simply mix the layers here. One for phpdoc comment, one for phpdoc type.

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.

Multiline PHPDoc types are, in fact, types "polluted" with whitespaces and *.

I wanted the simplest - in terms of changes - solution, feel free to propose an alternative with str_replace('\h', '[\h\v*]', ...).

Copy link
Copy Markdown
Contributor

@mvorisek mvorisek Jul 29, 2025

Choose a reason for hiding this comment

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

I mean to have a static method returning the regex with the solution you propose now and cached. This way I would be happy as the regex will remain untouched and someone else can impl. the idea from #8893 (comment) later.

To support my domain/layer claim, imagine the regex used in Markdown or // comments, in both cases ˙*˙ is wrong/not part of the type grammar.

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.

I am the author of the complex type regex and I say this is wrong, we would have context-dependent regex which is really not good. It is hack. I know I am not an official maintainer, I hope can I express change request for this.

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.

I say this is wrong

And I say it is right, which is as much as my opinion as yours is your opinion.

You say a lot of "should", "must", etc. - I like to stick to the facts, and those are:

  • no other proposition in over 3 years
  • new feature is supported
  • nothing gets broken

Copy link
Copy Markdown
Member

@keradus keradus Jul 29, 2025

Choose a reason for hiding this comment

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

for sake of solving issue from 2021, i'm OK to merge this as-is.
if you have even better proposal how to fix this @mvorisek , please raise follow-up PR, unless it would make things over-complicated. I think analysing "real content" without "*" could be more pure, but i'm afraid it would overcomplicate code without any benefit. happy to see the PR showing it's easy and elegant.

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.

I understand you guys but I simply view things differently. I work mostly on databases where integrity is important and no compromises are allowed. This tool is a piece of art and I probably should hand over my work on this topic to the next hands. Enjoy the summer! 🏝️

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i see value in your original idea, we simply need to get pragmatism when it's possible, otherwise the issue will stay open another 4 years. "Perfect is the enemy of good" and "we released in perfect way, just 5 years delayed"

@keradus keradus enabled auto-merge (squash) July 29, 2025 20:38
@keradus keradus merged commit e3c058b into PHP-CS-Fixer:master Jul 29, 2025
31 checks passed
@keradus keradus deleted the feat_PhpdocTypesFixer_multiline_array_shapes branch July 29, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add multiline array shapes support

4 participants