-
Notifications
You must be signed in to change notification settings - Fork 255
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
Automatically enable/disable Config::$backward_compatibility as needed for mods #8281
Automatically enable/disable Config::$backward_compatibility as needed for mods #8281
Conversation
Signed-off-by: Jon Stovell <[email protected]>
52e2ffd
to
374929e
Compare
If we do this, make it a decimal, mapping it to the lowest major.minor version we want to backward compat. If we choose to retain compat than a version or two, we can then trigger it, but not for the newest compat checks. Such as lets say we deprecate something for 3.1 and something else for 3.0, if we are in 3.0 mode, we would not trigger the compat code for 2.1, but only for 3.0. |
Good idea. That will match the way we track version info in the messages table, too. |
374929e
to
81ea204
Compare
Signed-off-by: Jon Stovell <[email protected]>
Signed-off-by: Jon Stovell <[email protected]>
81ea204
to
fe3c9fc
Compare
Look good now? |
... Wait. Maybe I misunderstood you. Were you actually suggesting that we change the type of Config::$backward_compatibility so that it contains a version number? If so, I think I don't agree. The point of Config::$backward_compatibility is to give a little bit of leeway for mod developers to update their mods while allowing admins to install the old version in the meantime. The idea is that if we make it easier for admins to update to SMF 3.0 right away, that will improve adoption rates, which in turn will (somewhat counterintuitively) motivate mod authors to update their mods more quickly. But that doesn't work if we extend backward compatibility support indefinitely. Instead, we'd just end up with an ever growing pile of hacks and workarounds that we'd need to maintain. |
I was thinking if it matched a version number, we could trigger specific compats. As mentioned, if we have it set to 3.0, we would enable any compat code required to run 3.0 mods. If it was set to 2.1, it would run any compat code for 2.1. Lets say we do not remove the 2.1 compat code when we release 3.1. We would be able to compat it still, but if we have no mods needing it, but a mod needs 3.0 compat, we would not trigger all the compat code needs. The code base itself would record what our minimum version we can compat is. Installing a mod that we can't compat would produce an error. |
Yeah, I'm afraid I don't like that idea. I really don't want us to have to keep maintaining the 2.1 compatibility code, and then 3.0 compatibility code, and then 3.1. compatibility code, and so on, indefinitely. It would become unsustainable, in the same way that our current upgrader wanting to support everything from Yabb on up has become unsustainable, except worse and way faster. |
The trouble with the idea is that it is very hard to simultaneously isolate the the backward compatibility code from the current code while also keeping it operable. I did as much as I could for that, but it took a lot of effort and it still isn't completely separate. If it were possible to slot backward compatibility code in like a plugin, that would be one thing. But right now it really isn't like that. Once we get to the kind of advanced, modern OO structure that @tyrsson is rightly advocating for as an eventual goal, then it will become much easier to maintain backward compatibility plugins like that. Compartmentalization is one of the great benefits of that kind of approach. But we aren't there yet, and by the time we are, the old procedural code from the 2.x days, and even the code from 3.0, will be long in the rearview mirror. |
When I first created the Config::$backward_compatibility setting during the initial OOP refactoring, I made it a hard-coded value with the idea that backward compatibility behaviours would be enabled by default in SMF 3.0, but then in 3.1 they would be disabled by default, and eventually removed entirely.
Ideally, though, we want to avoid the backward compatibility behaviours whenever possible. Since we only need them in order to support modifications that were written for SMF 2.1, it would be better if we only turned them on while such a mod is installed.
Therefore this PR does the following:
Makes $backward_compatibility a setting recorded in Settings.php rather than a hard-coded value in Config.php.
Adds a column to the log_packages table to record the SMF version that a package was installed for. If the package was installed using version emulation, then the emulated version will be recorded.
Allows the package manager to enable or disable the backward compatibility setting as needed for mods.
Changes the backward compatibility setting from a boolean to an integer so that admins can set it to 2 in order to force backward compatibility on even when no installed mods need it. This is intended for cases where a third party integration script (as opposed to an installed mod) expects the old SMF behaviour.
The logic that the package manager uses to decide whether to enable Config::$backward_compatibility is pretty simple. Every time a package is installed, upgraded, or uninstalled, the package manager will check the values of log_packages.smf_version for all installed packages. If any of them were installed by emulating SMF 2.1.x or below, then Config::$backward_compatibility will be turned on. If none were installed using emulation, then it will be turned off.
This also means that modification developers will not be able to update the content of their package_info.xml files to claim support for SMF 3.0 unless and until they have in fact rewritten it to properly support 3.0. This is a good thing.
@jdarwood007: If and when this is merged, we will need to update the new installer/upgrader code in #8093 to reflect the database changes in this PR. I will take care of that when the time comes, but I thought I should mention it now for the sake of good communication.