joomla-cms icon indicating copy to clipboard operation
joomla-cms copied to clipboard

[5.0] Remove the last code artefacts for Microsoft SQL Server / SQL Azure databases

Open richard67 opened this issue 3 years ago • 9 comments

Pull Request for Issue # .

Summary of Changes

This pull request (PR) removes the last code artefacts for using Joomla with Microsoft SQL Server / SQL Azure databases.

These database types are not supported anymore since Joomla 4.0, but there is the "SqlsrvChangeItem" class left in the "Joomla\CMS\Schema\ChangeItem" namespace, which is of no use in J4 and should be removed with 5.0.0.

For deprecation see PR #38428 .

This PR removes that class and corresponding code:

  • Remove file "SqlsrvChangeItem.php".
  • Remove alias for that file in "classmap.php".
  • Remove obsolete check for server type "mssql" in file "ChangeItem.php".

In addition, this PR removes an obsolete special handling for server type "mssql" in file "ChangeSet.php". The "sqlazure" subfolder doesn't exist anymore for core update SQL scripts since Joomla 4.0, and 3rd party extensions (which the database checker also checks since 4.0) are not expected to have any update SQL scripts for Microsoft SQL Server / SQL Azure.

All changes affect only the database checker ("System -> Maintenance -> Database", i.e. administrator/index.php?option=com_installer&view=database).

Additional information

The unit tests for the "Joomla\CMS\Schema\ChangeItem" namespace have never been migrated from Joomla 3 to Joomla 4, so in J4 we don't have any unit tests for the subject ot this PR.

I am preparing a PR to add unit tests for that namespace for the 4.2-dev branch (or maybe 4.3-dev), but that will still take some time until completed.

Testing Instructions

Code review, or make a new installation with the package built for this PR by drone and verify that the database checker works.

Actual result BEFORE applying this Pull Request

Obsolete code for Microsoft SQL Server / SQL Azure databases in the "Joomla\CMS\Schema\ChangeItem" namespace.

Expected result AFTER applying this Pull Request

No obsolete code for Microsoft SQL Server / SQL Azure databases in the "Joomla\CMS\Schema\ChangeItem" namespace.

The database checker works as well as without this PR.

Documentation Changes Required

None.

richard67 avatar Aug 06 '22 12:08 richard67

I have tested this item :white_check_mark: successfully on 96b73ca851aa4ed2a087ce257deb627eca648ef8

Database checker works after patch applied.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38405.

Abernyte-Git avatar Aug 06 '22 12:08 Abernyte-Git

LGTM but we'll need a deprecation tag on the class in the 4.x branch

@wilsonge I thought I had seen it there already, but you are right, it isn't. Will make a PR for that sooner or later.

richard67 avatar Aug 09 '22 12:08 richard67

LGTM but we'll need a deprecation tag on the class in the 4.x branch

@wilsonge I thought I had seen it there already, but you are right, it isn't. Will make a PR for that sooner or later.

@wilsonge Do I have to do that for the 4.3-dev or for the 4.2-dev branch?

richard67 avatar Aug 09 '22 13:08 richard67

Just clarified, will do it in 4.3-dev.

richard67 avatar Aug 09 '22 13:08 richard67

For deprecation see #38428 .

richard67 avatar Aug 09 '22 14:08 richard67

There are a few other places that refer to sqlserv that would be good to remove at the same time even if they are only comments

brianteeman avatar Aug 09 '22 15:08 brianteeman

There are a few other places that refer to sqlserv that would be good to remove at the same time even if they are only comments

@brianteeman Where? Could you point me to these? But if they are outside the schema namespace, I would do that with another PR.

Note that the database drivers still support to connect to MS SQL Server databases as external database. Only Joomla running on it is not supported anymore.

richard67 avatar Aug 09 '22 15:08 richard67

/administrator/components/com_joomlaupdate/src/Model/UpdateModel.php#L1173

/administrator/components/com_joomlaupdate/src/Model/UpdateModel.php#L1199

/administrator/components/com_menus/src/Model/ItemModel.php#L788

brianteeman avatar Aug 09 '22 15:08 brianteeman

@brianteeman /administrator/components/com_joomlaupdate/src/Model/UpdateModel.php#L1199 should be kept, otherwise the complete function would not make sense and should be removed. But we might keep the function for later use when we again remove support for some database type and need that to be checked before updating, and if we keep it we should keep the complete code so later readers can understand it.

richard67 avatar Aug 09 '22 16:08 richard67

I have tested this item :white_check_mark: successfully on 96b73ca851aa4ed2a087ce257deb627eca648ef8

I used the package built for this PR to test. DB checker works.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38405.

RickR2H avatar Oct 20 '22 08:10 RickR2H

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/38405.

RickR2H avatar Oct 20 '22 08:10 RickR2H

@richard67 can you please solve the conflicts please?

And maybe write the deprecation and removal documentation in manual? If not no problem I try to write it tomorrow.

HLeithner avatar Oct 21 '22 20:10 HLeithner

@HLeithner I've updated the branch and resolved the conflict. The failing appveyor is not related to this branch, it fails also in the 5.0-dev branch with the same error:

Error parsing appveyor.yml: (Line: 52, Col: 1, Idx: 1809) - (Line: 52, Col: 9, Idx: 1817): Duplicate key

Regarding deprecation docs: I don't know how to do that and won't learn that in the next days or weeks. So feel free to do that. But I do not see any need for any deprecations docs for the following reasons:

  • Before J4 the database schema checker only worked for the CMS core but not for 3rd party extensions.
  • Beginning with J4, including all pre-releases from 4.0.0-alpha1 on, the core does not support anymore to run on an MS SQL Server or MS Azure database.
  • Therefore it is impossible that the database schema checker ever has been used with such a database in J4, neither by the core nor by any 3rd party extension. So there is no need for any deprecation message. It absolutely makes no sense.

richard67 avatar Oct 21 '22 21:10 richard67

P.S.: It seems also drone is broken in the 5.0-dev branch. It fails with some error related to certificates.

richard67 avatar Oct 21 '22 21:10 richard67

Thanks

HLeithner avatar Oct 23 '22 07:10 HLeithner