Backward compatibility policy

[yguedidi]

Is there any established BC policy somewhere? Didn't manage to find it or I missed it.
Maybe it deserve to be documented? for end users and contributors.

To be honest I feel a bit confused regarding that.. It's not very clear to me so would be great to clarify :)

I recently proposed different changes, and some of them introduce some BC breaks indeed:

• Improve string support in Doctrine Annotations rector-src#3645 accepted idea but requested a BC layer that is hard to find as it's a bugfix to the Doctrine annotation parser
• Use exception for system errors rector-src#3739 rejected because changing a public API even if I believe it makes it more SOLID, and because no changes expected in that part of code to keep sync with other projects (that could deserve to be updated too maybe to benefit from the improvement)
• Allow to fully process files rector-src#3724 rejected partially because changing "way too much the original architecture"

Meanwhile, there were other accepted changes that are actual BC breaks for end users: rectors removal.
End user can have a committed configuration in their project using the rectors, and so on next update of Rector have an issue running it, see for example #7928

I find it a bit frustrating situation as a contributor too, and I'd like to avoid current and future contributors feel the same.
If there were an BC policy somewhere, defining what kind of change is accepted in a minor, what in a major, etc, it would help a lot!

So @TomasVotruba, do you already have such a policy in mind? or a draft? :)

My perspective as a contributor, be it right or wrong, is that I see Rector is at version 0.16 currently, so not yet a 1.0.0, so as per SemVer BC breaks are allowed in MINOR for 0.x versions.
I see this as a chance to improve Rector in the right direction on every topic (performance, architecture, use cases, required features, ...) before the release of a 1.0.0.
Being a widely used tool I guess, we must still pay attention to BC breaks and avoid them impact the end users I think.
Maybe here the solution is to start using version dedicated branches? have a 0.16 (or 0.16.x) branch for all backward compatible changes, and keep main be for 0.17.x-dev (for info PHPStan uses version branches)
Or if it's too much maintenance before an official 1.0.0, maybe embrasse BC breaks if they are for the good of Rector before 1.0.0 and tell users about it?

Really up to you, it's your project, you're the maintainer, you decide! :)
Goal here is really to clarify this missed point to avoid end user and contributor frustration

Thanks for reading

[TomasVotruba]

Hi,

in the past we added many rules that were niche to specific private project and tested out if it has potential to be used in public.
The more rules we have, the higher the complexity.

“Perfection is achieved, not when there is nothing more to add, but when there is nothing left to take away.”

That's why I'm reviewing rules that we use daily and bring value, or were just scratching left hand behind right ear a year ago. I've never used those once in any upgrade we're handling, neither being reported in the issues.

The goal now is to remove such rules and give more focus on useful ones for wide range like "add return/param/property type based on X", that bring value to everyone.

We're still in 0.x branch to allow these changes.

Next step is removal of next/prev/parent node connections and scope attribute, to ease the memory limits. The sooner we handle it, the sooner we'll achieve 1.x branch, with more stable API. Any help with those goal are welcomed 👍

[TomasVotruba]

Btw, I closed those PRs because of increasing complexity and changing the application workflow, not beacuse of BC.

I want to keep complexity as low as possible. Lowering is mostly welcomed, like removing scope and other attributes.
That's a way to go for inspiration :)

[yguedidi]

Thank you @TomasVotruba for your fast and kind answer! 🙂

First thing that pops in my mind when reading you is that rectorphp/rector-src#3645 could get accepted without a BC layer? 😁
Assuming of course that I commit to do the upgrade in other packages (which I commit to of course!).

Second, about complexity, maybe we don't see at the same place.
It's awesome that the current focus is on removing prev/next/parent/scope attributes, but on my side I don't feel confident enough (yet), to contribute to that part.
That's why most of my recent proposals are more about the end user experience, supporting more situations and cases (rectorphp/rector-src#3724, rectorphp/rector-src#3615, rectorphp/rector-src#3614, rectorphp/rector-src#3603, rectorphp/rector-src#3563, rectorphp/rector-src#3601, rectorphp/rector-src#3598, rectorphp/rector-src#3602)

Indeed those proposals require some architectural changes to be implemented, because current architecture doesn't support them.
And still being on 0.x is a chance to polish the architecture to me, not to have it freezed.
It will be much much harder to do architectural changes after 1.0.0 release..

To me those proposals were actually reducing the complexity, talking more precisely about rectorphp/rector-src#3739 here:

• following SOLID principle by giving a single responsibility to FileProcessorInterface, see Use exception for system errors rector-src#3739 (comment)
• use exceptions bubbling for errors and catch them at the right place
• de-duplicate common code between parallel and non parallel by extracting it in a service
• reduce pass by copy parameters.

But maybe you have another opinion there?
Maybe there are some concerns I'm not aware of that drive your decisions?
I'll be happy to learn from them.

I have identified some other places that could benefit from similar improvements, but again, you'll see them as architectural changes and probably don't accept... 😕

Finally, what would be the BC policy in few words? 🙂
My current understanding is that rules removal is OK but architectural changes are forbidden, even if needed to support new features 😅

Thanks for your patience

[TomasVotruba]

Well, it depends :) I don't have any clear definition that would fit any PR being merged or not.
I think the best way to learn is to go through the merged PRs and follow their pattern 👍

[yguedidi]

I think the best way to learn is to go through the merged PRs and follow their pattern 👍🏼

@TomasVotruba of course! I forgot to mention that I'm aware of that 🙂

Should I understand with that advice that you'd prefer me to stop trying to improve the architecture and start on the remove attributes project? to be honest my preference is for other changes that will help supporting better end user experience 😅

[TomasVotruba]

Again, it depends :) I cannot say what will be merged or not, untill the PR is clear and passing.

[yguedidi]

Noted! thank you

[yguedidi]

I will start opening RFC issues detailing my proposed changes, explaining why and benefits, before opening a PR, to start the discussion about them. 😉
But sometimes it's better to dive to code to validate the idea first, so I may often have something ready on my local when opening the issue 😅