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

Remove deprecated and empty FinalizeClassesWithoutChildrenRector + FinalizePublicClassConstantRector #5980

Merged
merged 3 commits into from
Jun 19, 2024

Conversation

TomasVotruba
Copy link
Member

@TomasVotruba TomasVotruba commented Jun 19, 2024

These 2 classes are empty and actually throw warning errors since 0.19.* release.
After couple of months, it's time for cleanup 👍

@TomasVotruba TomasVotruba merged commit a787d4c into main Jun 19, 2024
39 checks passed
@TomasVotruba TomasVotruba deleted the tv-depre branch June 19, 2024 15:37
@laoneo
Copy link

laoneo commented Jun 28, 2024

Should this not be done in a major release as it is a breaking change? Was surprised, that my script did throw an error when I did a patch update from 1.1.0 to 1.1.1.

@TomasVotruba
Copy link
Member Author

This was deprecated before 1.0, so it stayed longer than needed. It also didn't change any code and was empty to avoid causing bugs. The problem is that Rector didn't have any deprecation reporting during its run. There are couple deprecated rules now, but you would only notice them once removed. This will be fixed in next release: #6047

If you look for a replacement, see this tool that is safe and takes also Doctrine mappings into an account: https://tomasvotruba.com/blog/finalize-classes-automated-and-safe

@laoneo
Copy link

laoneo commented Jun 28, 2024

I still think that this should not be removed in a patch release if rector follows semver, even when the file got deprecated in a 0.x release. Anyway, the deprecation warning feature is nice as I was not aware of the deprecation of the file. Thanks for this.

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.

2 participants