Skip to content

Conversation

@calebdw
Copy link
Contributor

@calebdw calebdw commented Aug 12, 2025

Hello!

There are occasions where final classes still need to have protected methods/properties (e.g., Laravel Models and the attributes/scope methods, see larastan/larastan#2334). Instead of simply skipping all Models, it would be great if the specific methods/properties can be skipped.

This PR adds the ability for the user to define a callback that is called with the property/method and the class reflection and allows the user to determine if anything should be skipped

Thanks!

@calebdw calebdw force-pushed the calebdw/push-srmmoxrqrnqv branch 3 times, most recently from 6080ae1 to e7e268d Compare August 12, 2025 18:23
@TomasVotruba TomasVotruba self-requested a review August 12, 2025 18:37
@calebdw calebdw force-pushed the calebdw/push-srmmoxrqrnqv branch 2 times, most recently from fc92adf to e6ceac6 Compare August 13, 2025 05:08
@calebdw calebdw force-pushed the calebdw/push-srmmoxrqrnqv branch 2 times, most recently from 1208982 to bbfc125 Compare August 22, 2025 05:43
@calebdw calebdw force-pushed the calebdw/push-srmmoxrqrnqv branch from bbfc125 to 5afa462 Compare August 22, 2025 15:03
@TomasVotruba
Copy link
Member

Hey, I'm looking into this and adding callbacks to specific rule might trigger follow PRs with adding it everywhere.

Instead, this could be solved on a skip level. But performance would have to be considered first.
Still I'd like to keep the "skip" part of Rector as simple as possible.

In this specific example, the rule could be aware of the framework magic and behave propperly. We do the same with other rules, to take burden from the user having to spam the rector.php config to rule doing what it suppose to do.

Thanks for understanding 👍

@calebdw
Copy link
Contributor Author

calebdw commented Sep 4, 2025

Sounds good, I wasn't sure if y'all would be okay with framework specific logic in the rector itself

@calebdw calebdw deleted the calebdw/push-srmmoxrqrnqv branch September 4, 2025 11:34
@TomasVotruba
Copy link
Member

In moderate amount, yes 😉

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.

3 participants