Skip to content

Conversation

calebdw
Copy link
Contributor

@calebdw calebdw commented Aug 26, 2025

Hello!

Motivation

The contribution guidelines say to remove redundant tags that provide no additional information over native php types (note that this does not include tags that provide additional information such as descriptions or generics):

image

However, as can be seen by this PR, there are over 6,500 redundant tags in this codebase that are not needed and can be cleaned up. This PR adds the no_superflous_phpdoc_tags rule to the pint config to help enforce the contribution guidelines and clean up the redundant tags.

Removing redundant tags:

  • Enhances Code Readability: Removes redundant PHPDoc tags that duplicate information from PHP's native type hints, reducing visual clutter and making Laravel code easier to read.
  • Aligns with Modern PHP Practices: Encourages concise documentation by eliminating unnecessary @param or @return tags, aligning with Laravel’s emphasis on clean, expressive code.
  • Improves Developer Experience: Streamlines code reviews and maintenance by focusing PHPDoc comments on meaningful insights, such as complex logic or context-specific details.
  • Reduces Maintenance Overhead: Minimizes boilerplate documentation, allowing developers to focus on implementing and maintaining application functionality.

Review

Here's a breakdown of the commits:

  • ab6eeee This updates the pint config, fixes some stale tags (another reason why this rule is needed), and reworks the event helper so that the phpdocs are not detected as unused and removed
  • 95e65b2 this executes pint on the codebase which removes all the redundant tags

Thanks!

@taylorotwell
Copy link
Member

I'm tired, boss

@calebdw calebdw marked this pull request as draft August 26, 2025 19:01
@calebdw calebdw force-pushed the calebdw/push-oxnrrxttlsmz branch 2 times, most recently from 0a19556 to 83c0f81 Compare August 26, 2025 19:11
@calebdw calebdw marked this pull request as ready for review August 26, 2025 19:16
@browner12
Copy link
Contributor

Full strict typing in v13! 🤣

@yitzwillroth
Copy link
Contributor

yitzwillroth commented Aug 26, 2025

I'm very in favor of this, would be happy to help in the effort to review and/or break the PR into more manageable subsystem level PRs.

If we're going to do it, we need to keep the @throws tags and probably should consider keeping @param tags for arrays with PHPstan enforceable types.

@calebdw
Copy link
Contributor Author

calebdw commented Aug 26, 2025

we need to keep the @throws tags and probably should consider keeping @param tags for arrays with PHPstan enforceable types.

These are not redundant/superfluous, so Pint did not remove any of these. If you're referring to adding more native types then yes, we should keep any tags that are not redundant and add information that can't be expressed by native types alone like array shapes, generics, conditional returns, etc.

@yitzwillroth
Copy link
Contributor

Ah, yes, my bad. I saw a @param Throwable removable, read it as a @throws.

@taylorotwell taylorotwell marked this pull request as draft August 27, 2025 13:45
@taylorotwell
Copy link
Member

taylorotwell commented Aug 27, 2025

I'm not doing any of this on 12.x I don't think.

@calebdw
Copy link
Contributor Author

calebdw commented Aug 27, 2025

I can target master if you like

@crynobone crynobone added needs work Not quite ready for primetime and removed needs work Not quite ready for primetime labels Oct 1, 2025
@NickSdot
Copy link
Contributor

I can target master if you like

@calebdw I am pretty sure you are aware, but in case your are not here a friendly reminder that Taylor doesn't check replies in drafts.

I guess he expects you to target master, though. Maybe you want to do that?

@calebdw calebdw changed the base branch from 12.x to master October 14, 2025 17:38
@calebdw calebdw force-pushed the calebdw/push-oxnrrxttlsmz branch from 83c0f81 to e66ac17 Compare October 14, 2025 17:38
@calebdw calebdw changed the title [12.x] chore: remove redundant phpdocs [13.x] chore: remove redundant phpdocs Oct 14, 2025
@calebdw calebdw force-pushed the calebdw/push-oxnrrxttlsmz branch from e66ac17 to b148efd Compare October 14, 2025 17:44
@calebdw calebdw force-pushed the calebdw/push-oxnrrxttlsmz branch from b148efd to 95e65b2 Compare October 14, 2025 17:45
@calebdw
Copy link
Contributor Author

calebdw commented Oct 14, 2025

I've targeted master and the failing tests seem to be unrelated to the changes at hand

@calebdw calebdw marked this pull request as ready for review October 14, 2025 17:50
* Assign a set of tags to a given binding.
*
* @param array|string $abstracts
* @param mixed ...$tags
Copy link
Contributor

@NickSdot NickSdot Oct 15, 2025

Choose a reason for hiding this comment

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

Needs a second look.

@NickSdot
Copy link
Contributor

NickSdot commented Oct 15, 2025

Fixer Rule Context

The PHP CS Fixer no_superfluous_phpdoc_tags rule:

  • has no open issues
  • had it's last merged PR 16 months ago
  • is part of popular Fixer sets which makes it heavily used

So there is very little risk for major issues, IMO.

Crowd Review

Additionally, I manually reviewed the first 231 files with exactly one single finding which should get a second look. I added a comment for it.

Everything else:

  • aliases are handled correctly
  • no types removed that shouldn't
  • no other code changes found
image

Maybe others want to hop in and do the same to give Taylor some peace of mind. I assume if we get every component pre-reviewed by 2-3 people, he probably just could revert the Pint run commit and then re-run Pint without double checking everything in very detail.

Let's get this moved forward together. 🚀

Progress

The review feature here is not particular helpful for partial crowd reviews, so I start this table.

Component Reviewed By
Auth NickSdot
Broadcasting NickSdot
Bus NickSdot
Cache NickSdot
Collections NickSdot
Concurrency NickSdot
Conditionable NickSdot
Config NickSdot
Console NickSdot
Container NickSdot
Contracts NickSdot
Cookie NickSdot
Database -
Encryption -
Events -
Filesystem -
Foundation -
Hashing -
Http -
JsonSchema -
Log -
Macroable -
Mail -
Notifications -
Pagination -
Pipeline -
Process -
Queue -
Redis -
Routing -
Session -
Support -
Testing -
Translation -
Validation -
View -
Table as Markdown

| Component | Reviewed By|
| ---| ---|
Auth| NickSdot 
Broadcasting| NickSdot 
Bus| NickSdot 
Cache| NickSdot 
Collections| NickSdot 
Concurrency| NickSdot 
Conditionable| NickSdot 
Config| NickSdot 
Console| NickSdot 
Container| NickSdot 
Contracts| NickSdot
Cookie| NickSdot
Database| - 
Encryption | - 
Events | - 
Filesystem | - 
Foundation | - 
Hashing | - 
Http | - 
JsonSchema | - 
Log | - 
Macroable | - 
Mail | - 
Notifications | - 
Pagination | - 
Pipeline | - 
Process | - 
Queue | - 
Redis | - 
Routing | - 
Session | - 
Support | - 
Testing | - 
Translation | - 
Validation | - 
View | - 

I can either keep this table updated when others hop in, or Caleb can move it to the PR description.

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.

6 participants