search

unclecheese / silverstripe-display-logic

The Display Logic module allows you to add conditions for displaying or hiding certain form fields based on client-side behavior.
BSD 3-Clause "New" or "Revised" License
74 stars 71 forks source link

Future compatibility #48

Closed jonom closed 8 years ago

jonom commented 8 years ago

Hey @unclecheese,

Just tried installing SS 3.3 (dev) and noticed this happens:

  - Removing unclecheese/display-logic (1.3.1)
  - Installing unclecheese/display-logic (1.2.1)

Given that future minor releases of SS should be backwards compatible can I suggest that you set a minimum minor version requirement instead of a specific one? ^3.2 means >= 3.2.0 and < 4.0.0, so that could be a good option.

I can see why you might want to restrict people from installing this module with new versions of SilverStripe until you've tested with them, but if it means people can still install the module but are given an older version then it's probably counter-productive :)

unclecheese commented 8 years ago

You'd think so, but 3.1 -> 3.2 hosed this module, because CSS changes were not considered API changes. So I could go either way on this.

jonom commented 8 years ago

I thought that might be the case... I see the intention but couple of draw backs are

  1. harder to spot problems with this module during pre-release testing of new minor releases of SilverStripe (which would allow for a pre-emptive rather than reactive patch)
  2. instead of blocking the install an older version gets installed which is likely to be more broken

No. 1 isn't a strong argument on its own but when you consider no. 2 I think locking to a specific minor release probably is doing more harm than good at the moment. It might be a sensible strategy for a new module but since this one has legacy releases with looser version constraints it back-fires a bit. Even on a new module though I'm not sure I would like the approach as I would rather be able to install a module that might have some issues than not be able to get the module through composer at all.

Guess it's a bit of a limitation with Composer... would be nice to be able to retroactively apply a max version for a dependencies in previous releases, so we can be optimistic about future compatibility and only specify a max version once we know there's a problem.

We also might need to think more about how SemVer is applied outside of the PHP API too (such as in the markup and styling of the UI)... really if we're changing something that already exists, instead of introducing something brand new it should probably go in to master.

unclecheese commented 8 years ago

Yeah, I think I'm in agreement. a ^3.2 would be ideal. I suppose now the question is, is a change in the dependencies a major, minor, or point release, itself?

jonom commented 8 years ago

Good question. I would tend to think that dependency versions don't come in to it, especially if you're targeting a loose version as you can't know exactly what people are going to get. It's not actually bundled code so I don't think SemVer can apply to dependencies. I think we should treat dependencies simply as a flag that says "If you want to use this module you're also going to need this other module, somewhere in this version range".

The problem is that we're talking about inter-dependencies in an eco-system where multiple modules can require the same dependency. If dependencies were nested or 1:1 it wouldn't be a problem, but since they're not if modules target very specific versions it quickly leads to conflicts and when that happens I don't think you can actually resolve that conflict without forking one or many of the conflicting modules and either loosening up the requirements or changing the dependency requirements to match. I think it's a bit of a myth that you can run composer update and expect everything to work perfectly without testing. A developer should pay attention to how version numbers jump during updates and manually specify versions in their project composer.json if need be to ensure compatibility between all modules on a particular project. To me Composer makes it a lot easier to update dependencies but it's not a hands-off magic bullet system.

Bit of a long way of arriving at the point (heh... no pun intended) but I think a patch version release makes sense.

unclecheese commented 8 years ago

Ha. Fair enough. Yeah, really good points. I'll do this as a point release. Thanks for all your help, Jono!

unclecheese commented 8 years ago

Resolved in https://github.com/unclecheese/silverstripe-display-logic/commit/43a2213c4748c3f56fc91842ab3a4a3b22caacf0

jonom commented 8 years ago

No worries! Triggered some deep thinking about SemVer :)