RFC: Dropping support for SQL Server in 3.0 [accepted]

[Mark-H]

Request for comment

Background

For as long as I can remember, Revolution has supported an sqlsrv driver which allows it to be installed natively on windows servers using a Microsoft SQL Server rather than MySQL.

In that same time span, I've probably encountered like... 4 installations actually using MODX on sqlsrv, the latest being this thread on the forum last week where a user was trying to figure out why extras wouldn't show up, which is because those extras do not support (or do not claim to support) the sqlsrv driver.

Proposal

Given...

• There does not appear to be much, if any, real world usage of the sqlsrv driver
• Anecdotally I don't believe many (if any at all) core integrators and contributors actively test changes against sqlsrv, at most blindly copying schema changes as needed assuming things work
• Unit tests for each commit and pull request (in github actions) are only executed in a MySQL setup
• For MODX to be useful on sqlsrv, common extras also need to support for sqlsrv, which requires explicitly providing those schemas and models which I've very rarely seen in the real world (although extras that don't have a custom table are easier to have compatible).

... I'd like to invite feedback on dropping sqlsrv support entirely from MODX as of a future major release.

That future major release could very well be 3.0 if there's a clear consensus ahead of beta. That would still give enough time to cleanup the core, update the documentation, etc.

My opinion

Properly supporting sqlsrv would require a much more intensive effort from the entire community (integrators, contributors, and extras developers) to consistently test and making sure MODX and extras operate as they should on sqlsrv.

I do not believe that effort is worth it given the lack of users, so we might as well just stop pretending it's equally supported as mysql and drop sqlsrv support as of 3.0.

[rthrash]

This issue has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/rfc-on-dropping-sqlsrv-support/3784/1

[Ruslan-Aleev]

MODX needs to be cleaned of unused code for a long time. Related with - https://github.com/modxcms/revolution/issues/13199

[Mark-H]

It's not technically unused code if it's officially a supported feature though. ;) Hence proposing we drop it as a feature and get rid of it.

[Ibochkarev]

And what about PostgreSQL support. that would be a good innovation.

[Mark-H]

Postgres is not at all the question of this RFC. If you want to propose making that a core feature, then please open a separate issue to discuss that and to figure out what that would take to make happen. All I want to know in this issue from contributors and the wider community if they agree with the proposal to no longer support sqlsrv from 3.0 onwards or if there are important reasons to keep it that I'm missing.

I'm assuming 👍 and 🚀 responses to the opening post are votes in favor making it unanimous so far, but it has obviously only been 21 hours, so want to give more people the time to respond.

[BobRay]

Fine with me.

[JoshuaLuckers]

Sounds like a good idea to me!

[alroniks]

We have in code a lot of technical debt, and such things as sqlsrv sometimes do not allow us to move forward because need to think about implementation for such cases. Personally, I've never met the requirement of usage sqlsrv, so I've never added support of it to my extras. I absolutely agree about ridding of support of sqlsrv as a rarely used feature and focus on improving existing tools which used widely. Postgres is a bit off-topic, but it may be the next goal of improving MODX, including performance.

[smg6511]

Yes, I'd agree. I've not come across a need for sqlsrv support. I'd venture to guess that the open source audience that modx attracts overwhelmingly develops on the unix rather than windows platform.

[JoshuaLuckers]

@Mark-H I think we can move forward with this!

[alroniks]

How are we going to convert such RFCs to a solution or a made decision?

[Mark-H]

I hereby decree, without any particular power vested in me by anyone and definitely without any legal prejudice or authority, that the community and a sufficiently large set of active contributors have voted unanimously to drop support for sqlsrv in MODX 3 as proposed.

@alroniks How's that? 😄

--

Next step is obviously to work on actually removing it and making sure this is documented and well-communicated. I personally wont have time right away due to the bunch of security reports that came in over the past few days (all time I get for OS work, which is limited, will have to go into that first), but some initial thoughts on what now needs to be done:

• [ ] For 2.x, add a check in the config check dashboard widget. If installed on sqlsrv, show a warning their setup will not be supported in 3.x. While adding that, it may also be worthwhile to add a warning for "is core folder in the default location" as that's also an unsupported setup in 3.x that people should be alerted about. Perhaps someone like Ryan or Jay can help with the wording on this.
• [x] Add it into the docs. In the 2.x branch of the docs, add a note where sqlsrv is mentioned that it's dropped from 3.0. https://github.com/modxorg/Docs/pull/359 In the 3.x branch, add it to the getting started > upgrading to 3.x section. https://github.com/modxorg/Docs/pull/358
• [x] I don't think we currently have docs on how to migrate from one database engine to another, so that may also be useful to summarise. Never done that myself, but I'd imagine there are conversion tools of sorts available to point people to.
• [x] Actually, like, remove the thing from the 3.x branch. Removing the schema, sqlsrv-specific model, and sqlsrv-specific model build script/call (in composer.json). There might be sqlsrv-specific unit tests or processors that also need fixing. #15761
• [ ] Remove it from the setup as well, or perhaps replace disable sqlsrv where it's mentioned currently and replace it with a note that says its no longer supported. #15761 On an upgrade, check if the config currently specifies sqlsrv. If it does, error out and point to documentation on how to switch the database engine.
• [x] Perhaps post an announcement about this decision on modx.today and/or modx.com/blog? I can assist with the former. Post on MODX.today

@opengeek Do you have anything to add I may have forgotten about?

[garryn]

Just get rid of MSSQL support, I doubt it'd be fully functional right now anyway ... I don't think the sqlsrv stuff has been maintained for a while now. That's MHO.

[alroniks]

@alroniks How's that? 😄

Absolutely totally acceptable :)

[alroniks]

I will handle this during the next few weeks if nobody does mind.

[Ibochkarev]

Done #15761

[alroniks]

Not all points from https://github.com/modxcms/revolution/issues/15540#issuecomment-804594402 resolved. Please, don't close until it will be done.

[JoshuaLuckers]

For 2.x, add a check in the config check dashboard widget. If installed on sqlsrv, show a warning their setup will not be supported in 3.x. While adding that, it may also be worthwhile to add a warning for "is core folder in the default location" as that's also an unsupported setup in 3.x that people should be alerted about. Perhaps someone like Ryan or Jay can help with the wording on this.

If we add something like this in 2.x it means people need to update to a 2.x version where this is included first before updating to 3.x. Wouldn't it be better to have an extra that checks if you can upgrade your current 2.x install to 3.x? Or do both?

Remove it from the setup as well, or perhaps replace disable sqlsrv where it's mentioned currently and replace it with a note that says its no longer supported. On an upgrade, check if the config currently specifies sqlsrv. If it does, error out and point to documentation on how to switch the database engine.

What if they can't switch database engines? If you're in the process of upgrading MODX there is, unless you have a backup, no return because you copy the new files and replace the old ones. If you can't finish the upgrade procedure you end up with a broken install.

[Mark-H]

Spent some time today on docs and posted about it on MODX.today, so getting closer to close this issue.

What's still needed (IMO) is 2 things:

1. In the 2.x branch adjust the configcheck dashboard widget to check if sqlsrv is being used. If so, link them to https://docs.modx.com/3.x/en/getting-started/upgrading-to-3.0/sqlsrv (Similarly, check if the core path is in the default location and point them to https://docs.modx.com/3.x/en/getting-started/upgrading-to-3.0/core-folder if not, and perhaps also warn about PHP < 7.2?)
2. In the 3.x branch, make sure the installation halts early (pre-check) if the current configuration uses sqlsrv, linking those users to https://docs.modx.com/3.x/en/getting-started/upgrading-to-3.0/sqlsrv as well.

Could someone help with those changes? Shouldn't be a massive change but especially the setup bit might be a bit of digging to find the right place that has access to the current config and can still halt early.

If we add something like this in 2.x it means people need to update to a 2.x version where this is included first before updating to 3.x. Wouldn't it be better to have an extra that checks if you can upgrade your current 2.x install to 3.x? Or do both?

Not sure if people who don't update to the most recent 2.x version would realise there's an extra to check their 3.0 readiness.. I'm not against an extra being made that can help people determine what they'll need to change, but that should not replace the core checking its own requirements IMO.

[JoshuaLuckers]

Shouldn't be a massive change but especially the setup bit might be a bit of digging to find the right place that has access to the current config and can still halt early.

In setup the first step is to select the language, then you get to the next part where you can set/adjust the configuration key used for the setup.

The next step is "options" here you can select what you want to do; New Installation, Upgrade Existing Install, Advanced Upgrade Install. The value of the installmode variable is used to determine what option to check by default. E.g: https://github.com/modxcms/revolution/blob/e98caa93bb9311205602f335c9b0315e5fbff747/setup/templates/options.tpl#L26

In the options controller the installmode variable is set with the value returned by modInstall's getInstallMode method.

• modInstall::MODE_NEW
• modInstall::MODE_UPGRADE_REVO
• modInstall::MODE_UPGRADE_EVO
• modInstall::MODE_UPGRADE_REVO_ADVANCED

I propose to create a new constant modInstall::MODE_UPGRADE_SQLSRV_UNSUPPORTED and have modInstall::getInstallMode() check if $database_type is set to sqlsrv (similar to this).

We should make users aware they can't use "New Installation" or "Upgrade Existing Installation" if the install mode is equal to modInstall::MODE_UPGRADE_SQLSRV_UNSUPPORTED.

If a user already migrated from SQL Server to a MySQL database they might want to use "Advanced Upgrade Install" to edit the database config.