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

Backwards compatibility promise #8748

Closed
pamil opened this Issue Sep 28, 2017 · 6 comments

Comments

Projects
None yet
5 participants
@pamil
Member

pamil commented Sep 28, 2017

Sylius has gone stable, meaning no BC breaks should happen until next major release. But first, we have to define what a BC break is.

There's the one thing I know for sure - we can't follow semantic versioning strictly. Sylius has too many responsibilities for that: a set of decoupled components and Symfony bundles, an e-commerce framework, and the shop application itself.

The ones that have been with us since 0.x releases already know that the effort needed to upgrade Sylius to a next version had been significantly reduced for beta releases; keeping this trend is a must!

We can think of Symfony BC promise as the exemplary one - and in fact most of it will be valid for Sylius as well. I'm including my proposed changes to that BC policy below:

  • Actions required after each Sylius update:
    • running schema migrations
    • clearing & warming up cache
  • Model interfaces:
    • methods can be added
    • optional arguments might be added to methods
  • Models:
    • database structure can be modified - adding new fields / relations, etc. (by using migrations)
    • default values must be provided for new fields if they are not nullable
    • provides backwards compatibility layer for model interfaces - extending these models makes changing model interfaces not affecting end-user
  • Routing:
    • route names cannot change
    • routes can be added (with sylius_ prefix)
    • optional arguments might be added to routes
  • Services:
    • names cannot change
  • Event listeners / subscribers:
    • no BC policy on them
    • only for storing simple logic, relying on other services
  • Templates:
    • events cannot be deleted
    • templates cannot be deleted
    • blocks cannot be deleted
  • Behat
    • no BC policy yet - should be added in 1.1 / 1.2, including steps and context services names
  • Javascript code
    • no BC policy

Please treat the above as an initial draft, just some thoughts off the top of my head. I would love to hear your opinions on this subject and combine all of them into a BC policy that is suitable both for maintainers, plugin creators, Sylius developers and end-users.

@pamil pamil added the RFC label Sep 28, 2017

@pamil pamil added this to the 1.0 milestone Sep 28, 2017

@gabiudrescu

This comment has been minimized.

Show comment
Hide comment
@gabiudrescu

gabiudrescu Sep 28, 2017

Contributor

@pamil can you point out some explicit Semver rules that Sylius is planning not to enforce?

Contributor

gabiudrescu commented Sep 28, 2017

@pamil can you point out some explicit Semver rules that Sylius is planning not to enforce?

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 28, 2017

Member

@gabiudrescu that's a good point that should be clarified. Semver states that 1.0.0 release defines the public API.

When we restrict the public API to some point (eg. event listeners are not a part of it, model interfaces shouldn't be implemented standalone without extending model class if you want backwards compatibility, etc.), it covers the e-commerce framework part.

There's also the application part, meaning that changes made outside of the public API might in fact cause some failures, so that every update requires some manual supervision.

Member

pamil commented Sep 28, 2017

@gabiudrescu that's a good point that should be clarified. Semver states that 1.0.0 release defines the public API.

When we restrict the public API to some point (eg. event listeners are not a part of it, model interfaces shouldn't be implemented standalone without extending model class if you want backwards compatibility, etc.), it covers the e-commerce framework part.

There's also the application part, meaning that changes made outside of the public API might in fact cause some failures, so that every update requires some manual supervision.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Sep 28, 2017

Contributor

can you point out some explicit Semver rules that Sylius is planning not to enforce?

At a minimum it looks like the model interfaces changes would fall under that, adding methods to an interface can be argued as a B/C break. Twig got around this by using new interfaces for new methods, which can be mildly annoying when checking an object's compatibility with something (not saying I'd advocate for that at all, but it does technically get around the B/C concern).

Contributor

mbabker commented Sep 28, 2017

can you point out some explicit Semver rules that Sylius is planning not to enforce?

At a minimum it looks like the model interfaces changes would fall under that, adding methods to an interface can be argued as a B/C break. Twig got around this by using new interfaces for new methods, which can be mildly annoying when checking an object's compatibility with something (not saying I'd advocate for that at all, but it does technically get around the B/C concern).

@FabianWesner

This comment has been minimized.

Show comment
Hide comment
@FabianWesner

FabianWesner Sep 28, 2017

Hi Kamil, some comments on this.

(1) You assume that the customer run db migrations after every update so that you can do db schema changes even in minor releases. This makes sense for practical reasons, but this way you will loose one of the main benefits of semantic versioning. Only when people can trust that nothing breaks in a minor or patch release, they are willing to use the Caret Version Range (^) in their composer.json. But when there may be db schema changes here and there, they will look down the minor version as well (e.g. ~1.2.0) or even version a concrete version. A db schema change always means risk and maybe effort for a project, even when the new field is optional. But db schemas may cause downtimes and they may take a lot of time when tables are big. So no serious business is willing to take them without proper testing in advance. Most of your minor releases will not have schema changes I guess, but this way you make them all suspicious. This is a hard problem that normal PHP frameworks don't have.

(2) About models: changes in the public interface are obvious but sometimes you have internal changes that change the contract of a public method. This happens sometimes when you cleanup code. For instance if your registerCustomer() method sends an email and someone removes it from here because its simply the wrong place. This does not change the signature in the interface because the name of the method and the parameters do not change but obviously this is a BC break.

(3) I don't know exactly how your translation system works, but you should check what happens when you add a new text output. E.g. when "a voucher code cannot be applied" comes from the inside of the system then this will result in a BC break, because people have to translate this one to their locales before deployment.

Best

Fabian

FabianWesner commented Sep 28, 2017

Hi Kamil, some comments on this.

(1) You assume that the customer run db migrations after every update so that you can do db schema changes even in minor releases. This makes sense for practical reasons, but this way you will loose one of the main benefits of semantic versioning. Only when people can trust that nothing breaks in a minor or patch release, they are willing to use the Caret Version Range (^) in their composer.json. But when there may be db schema changes here and there, they will look down the minor version as well (e.g. ~1.2.0) or even version a concrete version. A db schema change always means risk and maybe effort for a project, even when the new field is optional. But db schemas may cause downtimes and they may take a lot of time when tables are big. So no serious business is willing to take them without proper testing in advance. Most of your minor releases will not have schema changes I guess, but this way you make them all suspicious. This is a hard problem that normal PHP frameworks don't have.

(2) About models: changes in the public interface are obvious but sometimes you have internal changes that change the contract of a public method. This happens sometimes when you cleanup code. For instance if your registerCustomer() method sends an email and someone removes it from here because its simply the wrong place. This does not change the signature in the interface because the name of the method and the parameters do not change but obviously this is a BC break.

(3) I don't know exactly how your translation system works, but you should check what happens when you add a new text output. E.g. when "a voucher code cannot be applied" comes from the inside of the system then this will result in a BC break, because people have to translate this one to their locales before deployment.

Best

Fabian

@pamil

This comment has been minimized.

Show comment
Hide comment
@pamil

pamil Sep 29, 2017

Member

@mbabker using standalone interfaces to introduce new methods sounds good, especially for services like repositories, factories etc. but models are kind of special. If we introduce a new model interface as an addition to the existing one, then people would need to add mapping, modify and extend their models, set up validation etc.

@FabianWesner that's an extensive comment, thank you!

(1) Running migrations of each update doesn't sound practical for the end-users indeed, so I think we might restrict it to minor releases only (happening every 3-6 months, not sure yet). Then, if someone uses 1.0.0:

  • any patch release update (1.0.x) will be same and won't require any extra work, no migrations etc. and they would get the latest bugfixes safely
  • any minor release update (1.x.x) will require some work, but there will be new, shiny features like #8676, so it would make putting this work more attractive - and if they wouldn't be encouraged enough, they can stay on the current minor and still receive latest bugfixes

(2) If a method has side-effects, we can't remove them in minors, that's right.

(3) We use CrowdIn for translations (http://translate.sylius.org) and I'm just setting up support for both existing branches, so that translators should have enough time to work on these and minor releases will contain these new translations. By the way, we always have 100% coverage on English translations, so if there aren't any in target language, we fallback to the English one (or the ones specified by end-user).

Member

pamil commented Sep 29, 2017

@mbabker using standalone interfaces to introduce new methods sounds good, especially for services like repositories, factories etc. but models are kind of special. If we introduce a new model interface as an addition to the existing one, then people would need to add mapping, modify and extend their models, set up validation etc.

@FabianWesner that's an extensive comment, thank you!

(1) Running migrations of each update doesn't sound practical for the end-users indeed, so I think we might restrict it to minor releases only (happening every 3-6 months, not sure yet). Then, if someone uses 1.0.0:

  • any patch release update (1.0.x) will be same and won't require any extra work, no migrations etc. and they would get the latest bugfixes safely
  • any minor release update (1.x.x) will require some work, but there will be new, shiny features like #8676, so it would make putting this work more attractive - and if they wouldn't be encouraged enough, they can stay on the current minor and still receive latest bugfixes

(2) If a method has side-effects, we can't remove them in minors, that's right.

(3) We use CrowdIn for translations (http://translate.sylius.org) and I'm just setting up support for both existing branches, so that translators should have enough time to work on these and minor releases will contain these new translations. By the way, we always have 100% coverage on English translations, so if there aren't any in target language, we fallback to the English one (or the ones specified by end-user).

@januzis

This comment has been minimized.

Show comment
Hide comment
@januzis

januzis Oct 6, 2017

What is the purpose of model interfaces, if you have to depend on the model classes themselves, in order to be sure nothing brakes between minor versions? Surely it would make more sense to depend on the model classes directly then.

My vote for no model interface changes with patch versions, and only adding extra fields/methods with minor versions, so that the people who have their own model interface implementations, could at least update to the latest patch version without worries.

januzis commented Oct 6, 2017

What is the purpose of model interfaces, if you have to depend on the model classes themselves, in order to be sure nothing brakes between minor versions? Surely it would make more sense to depend on the model classes directly then.

My vote for no model interface changes with patch versions, and only adding extra fields/methods with minor versions, so that the people who have their own model interface implementations, could at least update to the latest patch version without worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment