Skip to content
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

feat: adjust AddGenericReturnTypeToRelationsRector rule to be able to generate new generic style code #263

Merged
merged 2 commits into from
Oct 26, 2024

Conversation

canvural
Copy link
Contributor

@canvural canvural commented Oct 24, 2024

This PR adjusts AddGenericReturnTypeToRelationsRector rule and makes it configurable to also work with new relation generics introduced in Laravel 11.15

Fixes #259

@peterfox
Copy link
Collaborator

Making it a configurable rule is a bit of a pain as it will break people's Rector configs if they have the rule applied. A better approach would be to detect the Laravel version installed. I appreciate that there's not a class for this, but it wouldn't be too hard to implement.

@GeniJaho
Copy link
Collaborator

If we have the false value as the default, it won't break existing configurations. However, users would need to know exactly when to update the Rector config to use the rule as a configurable one. The changes won't take effect otherwise.

I think we can merge the PR with only a change to give the configuration a default value of false, so existing configs won't break, and then figure out in another PR how to make the Rule check the version in Composer. These two PRs should probably be part of the same release. 👀

@peterfox
Copy link
Collaborator

@GeniJaho I agree. Let's merge it.

The trick wouldn't be to use composer. I believe a const value exists in the Laravel framework's kernel we could use PHPStan to grab the version. Writing tests for this would be challenging. We'd have to consider how we do CI builds in a matrix to make it work.

@canvural canvural force-pushed the laravel-11-relation-generic-types branch from 8b81d57 to 6d37120 Compare October 26, 2024 20:21
@canvural
Copy link
Contributor Author

Yeah I thought about auto detecting the Laravel version, but couldn't find an easy way to do so. And not sure anymore if we even should do it. It's kinda users responsibility.

@GeniJaho I agree. Let's merge it.

The trick wouldn't be to use composer. I believe a const value exists in the Laravel framework's kernel we could use PHPStan to grab the version. Writing tests for this would be challenging. We'd have to consider how we do CI builds in a matrix to make it work.

Are you referring to this? We'd need to detect the autoloader of the project and then ask PHPStan about this class. Maybe Rector itself has some helper class already for this case 🤔


I updated the PR with making the configuration optional, and default to false.

@GeniJaho GeniJaho merged commit a54acd5 into driftingly:main Oct 26, 2024
5 checks passed
@peterfox
Copy link
Collaborator

Rector already loads the code via PHPStan. So you only need to use Reflection to grab the Version.

@canvural canvural deleted the laravel-11-relation-generic-types branch October 27, 2024 08:42
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.

AddGenericReturnTypeToRelationsRector not compliant with updated relation generic
3 participants