0017 - Add our backward compatibility promise

[PierreRambaud]

Questions	Answers
Description?	Try to explain our backward compatibility promise

Votes

Vote

Maintainer	Vote (✅ or ❌)
@atomiix
@eternoendless	✅
@jolelievre	✅
@kpodemski	✅
@matks	✅
@matthieu-rolland	✅
@NeOMakinG	✅
@PululuK
@sowbiba

[matks]

Some first comments :) but nice job

[jolelievre]

Hmmm this is interesting ^^

I commented most of my worries, the biggest one being the possibility of changing services constructor parameters. That said if we formalize the use of the @internal tag and use it extensively (meaning we would need to go through all our services before releasing the v8 to clearly state which are public and which are not) then it might limit my worries

If we do so I would add a breaking change: Turn "public" code into "internal" one

[matks]

Hi @PierreRambaud there is some things confusing between sections 😅 is this because you are right now re-writing the ADR?

[MeKeyCool]

I agree, IMO CQRS philosophy asks more attentions to command than queries. Queries can easily be duplicated cause they don't impact data and application as deep as commands.

[matks]

@PierreRambaud Would you like that I continue applying feedbacks on this ADR?

[PierreRambaud]

@matks Of course! Do whatever you want with this :)

[Progi1984]

Some feedbacks

[matks]

@eternoendless @Progi1984 @atomiix @jolelievre I updated the ADR please read again

[Progi1984]

There are some feedbacks.

[matks]

I think this is a good first version, enough for our current needs. We can update it as we find new usecases.

[eternoendless]

I agree with @matks

I think we can adopt this first version and add (or remove) stuff as needed in the future.

[jolelievre]

I have a few suggestions, but basically agree to the ADR I don't know if this should be fixed before merging?

[jolelievre]

I think both tags have a different meaning even if they imply the same concept that we don't promise anything:

• @experimental is some code still under development that may change until it's officially released
• @internal is a released production code, that is technically extendable (no final) because we may need it internally, but is used for internal purposes and can be modified at will

I think we should detail the difference somewhere, probably a dedicated paragraph for internal

[eternoendless]

I think internal components could (and probably should) be described in a new ADR. Why don't you submit a proposal?

[matthieu-rolland]

I have a question regarding the form simplification PRs we have...

Is changing the template of a form using form_widget instead of rendering it field by field... considered a BC Break even if it doesn't change the HTML in the end (because all classes, wording etc are transcribed in the form type)

I would say no but I'd like some other dev's input here 🤔

[jolelievre]

I have a question regarding the form simplification PRs we have...

Is changing the template of a form using form_widget instead of rendering it field by field... considered a BC Break even if it doesn't change the HTML in the end (because all classes, wording etc are transcribed in the form type)

I would say no but I'd like some other dev's input here 🤔

Since the layout doesn't change I don't think it's a BC break it's more like refacto: a different way to attain the same result

I also have one last question: so with this ADR we allow, for example, to change a service constructor's parameter when it's built by the Symfony DI. So our policy allows to do it (also we should try to limit such changes as much as possible), but does it mean we don't need to flag the PR as BC break and not document it? Even if the policy allows it it's probably a good thing to document and warn the community.

Similar question regarding the experimental code (tagged with @experimental) this time however I don't think it's relevant to flag PRs or to document such code modification since the code is by essence destined to be modified until it's still considered experimental.

Maybe those two questions could be answered in a dedicated ADR that comes as an appendix for this one? I'm not sure if the BCbreak documentation is really an architecture decision, it's more about the Project management but if it's not defined here I'm not sure where it should be? 🤷‍♂️ If not in this repo then maybe in this one?https://github.com/PrestaShop/open-source

[matthieu-rolland]

I also have one last question: so with this ADR we allow, for example, to change a service constructor's parameter when it's built by the Symfony DI. So our policy allows to do it (also we should try to limit such changes as much as possible), but does it mean we don't need to flag the PR as BC break and not document it? Even if the policy allows it it's probably a good thing to document and warn the community.

I think if people use the services definitions properly, they won't have to worry about the modifications made in the constructor. If it's a problem for them they are probably doing something wrong like instantiating it manually or doing an override manually... and marking it as a warning would validate this wrong behavior...

But maybe I didn't think of a legitimate use case ?

[matks]

I also have one last question: so with this ADR we allow, for example, to change a service constructor's parameter when it's built by the Symfony DI. So our policy allows to do it (also we should try to limit such changes as much as possible), but does it mean we don't need to flag the PR as BC break and not document it? Even if the policy allows it it's probably a good thing to document and warn the community.

I think people do not care about labels on the PR. What they care about is the list of BC breaks in the devdocs. They will not check every single PR for each BC break.

I think we could mark these modifications in a dedicated section on devdocs 8.0.0 BC breaks page, we can call them 'things that look like BC breaks BUT actually they're not' 😆 . The idea is to provide the information and let users choose what to do with it.

[FabienPapet]

useless change :)

[PierreRambaud]

Could you please go a little bit faster with this one, I need to clean up my GitHub forks :)

[matks]

@PierreRambaud Sorry about delay 😓 2 weeks ago we were close to be able to close the topic with a group agreement however we found some little funny thing and it got delayed

[PierreRambaud]

I give you one more week, but maybe you can merge it and keep the ADR in discussion.

[eternoendless]

We have 6 approvals, I think we agree on the basics, and I see no fundamental dissent in the discussion. Details can be ironed out in documentation, and any further decisions can be recorded in future ADRs. In the spirit of lazy consensus, I'm calling this ADR as accepted.