Skip to content

Conversation

@richard67
Copy link
Member

@richard67 richard67 commented Aug 6, 2022

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.

@Abernyte-Git
Copy link

I have tested this item ✅ successfully on 96b73ca

Database checker works after patch applied.


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

@richard67 richard67 requested a review from wilsonge August 6, 2022 13:13
Copy link
Contributor

@wilsonge wilsonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@richard67
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

richard67 commented Aug 9, 2022

Just clarified, will do it in 4.3-dev.

@richard67
Copy link
Member Author

For deprecation see #38428 .

@brianteeman
Copy link
Contributor

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

@richard67
Copy link
Member Author

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.

@brianteeman
Copy link
Contributor

/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

@richard67
Copy link
Member Author

@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.

@RickR2H
Copy link
Member

RickR2H commented Oct 20, 2022

I have tested this item ✅ successfully on 96b73ca

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
Copy link
Member

RickR2H commented Oct 20, 2022

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 20, 2022
@HLeithner
Copy link
Member

@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.

@richard67
Copy link
Member Author

@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
Copy link
Member Author

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

@HLeithner HLeithner merged commit 2f1ae96 into joomla:5.0-dev Oct 23, 2022
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 23, 2022
@HLeithner
Copy link
Member

Thanks

@HLeithner HLeithner added this to the Joomla! 5.0 milestone Oct 23, 2022
richard67 added a commit to richard67/joomla-cms that referenced this pull request Mar 11, 2023
HLeithner pushed a commit that referenced this pull request Mar 11, 2023
…remove 4.x update SQL scripts (#40083)

* Init deleted files and folders for 5.0

* Add deleted files and folders from #38406

* Add deleted file from #38405

* Add deleted files and folders from #39134

* Init renamed files for 5.0

* Add deleted and renamed files and folders from dependency updates

* Fix alpha odering of deleted files

* Remove j4 update SQL scripts and add initial one for j5

* Use real DDL in update SQL script
@richard67 richard67 deleted the 5.0-dev-remove-sqlsrv branch March 22, 2023 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants