? Pending

User tests: Successful: Unsuccessful:

avatar richard67
richard67
14 Aug 2020

Pull Request for Issue #19348 .

Summary of Changes

This Pull Request (PR) reverts the removal of column custom_data from the extensions table made once with PR #14750 , as requested by extension developers in the referenced issue.

Testing Instructions

Test 1: Reproduce the issue

  1. Have a current 4.0-dev branch or latest nightly build, either a new installation or one updated from 3.10, whatever you just have, and if you have nothing just install one.

  2. With a tool like e.g. phpMyAdmin (MySQL or MariaDB) or phpPgAdmin (PostgreSQL), check if column custom_data of the extensions table exists.

Result: See section "Actual result BEFORE applying this Pull Request" below. The column doesn't exist.

Test 2: New installation with the patch of this PR applied

This test shall verify that that the column custom_data of the extensions table is there after a new installation, and that installing extensions of any kind works.

  1. Apply the patch of this PR on a current 4.0-dev branch or latest nightly build and make a new installation, or use the installation package which has been built for this PR to make a new installation.

You can find a page with a link to an installation package for download as shown in the following screenshot with the green ellipse (if necessary expand the "Show all checks" to see that):
pr-downloads

  1. After the installation has finished, login to backend ang to to "System - Information - Database. Check if there are database problems shown.

Result: If you have used the installation package which has been built for this PR, there is only one database problem about not matching versions shown, otherwise no database problem is shown.

  1. With a tool like e.g. phpMyAdmin (MySQL or MariaDB) or phpPgAdmin (PostgreSQL), check if column custom_data of the extensions table exists.

Result: The column exists.

  1. Install extensions of different types, whatever you can find. Possible types are:
  • Component
  • File
  • Language
  • Library
  • Module
  • Package
  • Plugin
  • Template

I'm not sure if you can find extensions of all of these types which are ready for J4, but test whatever you can, the more, the better.

Result: No SQL errors about column custom_data not having a default value. The PHP added back with this PR prevents this.

Test 3: Update a 3.10 to a 4 which has the patch of this PR applied

This test shall verify that that the column custom_data of the extensions table is still there after updating a 3.10.

  1. Update a current 3.10-dev or 3.10 nightly to a 4.0-dev which contains the patch of this PR.

You can find a page with a link to a custom update URL and an update package for download at the same place as described in thep 1 or the previous test 1 for the installation package.

Result: The update suceeds without SQL errors.

  1. With a tool like e.g. phpMyAdmin (MySQL or MariaDB) or phpPgAdmin (PostgreSQL), check if column custom_data of the extensions table exists.

Result: The column exists.

Test 4: Update a 4 without this patch to a 4 which has the patch of this PR applied, including a manual pre-update procedure

This test shall verify that the manual pre-update procedure to fix updating e.g. from 4.0 Beta-3 to a future Beta-4 which will contain the patch of this PR is sufficient to make the update succeed.

  1. Manual pre-update step: On an installation of a current 4.0-dev branch or latest nightly build which does not contain the patch of this PR, use a tool like e.g. phpMyAdmin (MySQL or MariaDB) or phpPgAdmin (PostgreSQL) to run following SQL commands, having replaced #_ by the database prefix of your installation.

On MySQL or MariaDB databases:

ALTER TABLE `#__extensions` ADD COLUMN `custom_data` text NOT NULL;

On PostgreSQL databases:

ALTER TABLE "#__extensions" ADD COLUMN "custom_data" text NOT NULL;
  1. Update this installation to a newer 4.0-dev which contains the patch of this PR.
    See step 1 of test 1 about where to find a custom update URL or update package.

Result: The update suceeds without SQL errors.

  1. With a tool like e.g. phpMyAdmin (MySQL or MariaDB) or phpPgAdmin (PostgreSQL), check if column custom_data of the extensions table still exists.

Result: The column still exists.

  1. Install extensions of different types, whatever you can find. See step 4 of test 2 for possible extension types.

Result: No SQL errors about column custom_data not having a default value. The PHP added back with this PR prevents this.

Actual result BEFORE applying this Pull Request

Colum custom_data of the #__extensions table is removed when updating 3.10 to 4 and not existing on a new installation of 4.

Expected result AFTER applying this Pull Request

Colum custom_data of the #__extensions table is not removed when updating 3.10 to 4, and it is existing on a installation of 4.

Updating a previous 4 Beta version to a future one which contains this PR will require a manual step for other reasons (see e.g. #30333 #30363 #30365 for details). After this manual step, colum custom_data of the #__extensions tables will be back, and after the update it will be initialized properly when installing any kind of extension.

Documentation Changes Required

We haven't mentioned the removal of this column in https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4, as far as I can see, so no need to change something there.

But in the release notes of a future J4 Beta which will include this PR we need to describe the manual step, see "Test 4" in the testing instructions. In additon it might need other manual steps for another issue not related to this PR, see e.g. #30369 .

avatar richard67 richard67 - open - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 14 Aug 2020
Category SQL Administration com_admin Postgresql Installation
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar wilsonge
wilsonge - comment - 14 Aug 2020

I know you know - but so it's in this issue rather than glip - we'll also need to add back some of the PHP from #14750 that got removed too I guess

avatar richard67
richard67 - comment - 14 Aug 2020

I see. Will check later.

avatar richard67
richard67 - comment - 14 Aug 2020

@wilsonge But it doesn't need to add back the column to the unit tests, does it?

avatar wilsonge
wilsonge - comment - 14 Aug 2020

Depends, currently that column is marked as NOT NULL so IF those tests still exist it may need adding else you're going to get an error from SQL i guess?

avatar richard67 richard67 - change - 14 Aug 2020
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 14 Aug 2020
Category SQL Administration com_admin Postgresql Installation SQL Administration com_admin Postgresql Installation Libraries
avatar richard67
richard67 - comment - 14 Aug 2020

@wilsonge

Depends, currently that column is marked as NOT NULL

Yes, and we shouldn't add a default value because it's a column of type text.

so IF those tests still exist it may need adding else you're going to get an error from SQL i guess?

Yes. But I can't find those tests in the CMS in 4.0. Could you give me a hint? Or are they coming from the framework now?

avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
Title
[4.0] [WiP] Revert removal of the "custom_data" field of the "extensions" database table.
[4.0] Revert removal of the "custom_data" field of the "extensions" database table.
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67
richard67 - comment - 14 Aug 2020

Wait, I just notice I have to fix something. Insert statements into the extensions table in SQL scripts running after the ones modified by this PR need tio be extended by the column and values for it because the column doesn't have and shall not have a default value. Text type columns shall not have a defauls in MySQL 8.

avatar richard67 richard67 - change - 14 Aug 2020
Title
[4.0] Revert removal of the "custom_data" field of the "extensions" database table.
[4.0] [WiP] Revert removal of the "custom_data" field of the "extensions" database table.
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar joomla-cms-bot joomla-cms-bot - change - 14 Aug 2020
Category SQL Administration com_admin Postgresql Installation Libraries SQL Administration com_admin Postgresql
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67 richard67 - change - 14 Aug 2020
Title
[4.0] [WiP] Revert removal of the "custom_data" field of the "extensions" database table.
[4.0] Revert removal of the "custom_data" field of the "extensions" database table.
avatar richard67 richard67 - edited - 14 Aug 2020
avatar richard67
richard67 - comment - 14 Aug 2020

Drone failure is unrelated.

Now finally ready for testing.

avatar wilsonge
wilsonge - comment - 14 Aug 2020

Largely looks good to me

We’re still using an empty string for 3rd party extensions. I also ask because the core extensions update scripts will have the empty json string but if they are discover installed from a 3.x update then it will be an empty string. Is it not better to use empty json there too?

Also because you’ve added the property into the base table class I assume we no longer need it in the adapters.

avatar SharkyKZ
SharkyKZ - comment - 14 Aug 2020

Keep using empty string. The column may not necessarily be used for JSON.

Insert statements in install SQL need to be updated.

avatar richard67
richard67 - comment - 14 Aug 2020

@SharkyKZ For the custom_data I use consistently empty string. The change to empty json I’ve made at some places for the „params“ column, which at the most places is json but at a few places empty string. Please review this again and confirm.

Regarding the insert statements in the installation SQL you are right, I will add that tomorrow.

avatar richard67 richard67 - change - 15 Aug 2020
Title
[4.0] Revert removal of the "custom_data" field of the "extensions" database table.
[4.0] [WiP] Revert removal of the "custom_data" field of the "extensions" database table.
avatar richard67 richard67 - edited - 15 Aug 2020
avatar richard67 richard67 - change - 15 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 15 Aug 2020
avatar richard67 richard67 - change - 15 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 15 Aug 2020
avatar richard67 richard67 - change - 15 Aug 2020
Title
[4.0] [WiP] Revert removal of the "custom_data" field of the "extensions" database table.
[4.0] Revert removal of the "custom_data" field of the "extensions" database table.
avatar richard67 richard67 - edited - 15 Aug 2020
avatar richard67 richard67 - change - 15 Aug 2020
The description was changed
avatar richard67 richard67 - edited - 15 Aug 2020
avatar richard67
richard67 - comment - 15 Aug 2020

@steffans Do you have a build of your YOOtheme Pro theme available which can be installed on J4 and uses the custom_data field, or can you easily prepare suchg a build? If so: Could you use that for testing this pull request here, checking that saving data in that field works, in addition to what the testing instructions say? That would be very helpful. Thanks in advance.

avatar richard67
richard67 - comment - 15 Aug 2020

@SharkyKZ Done. I've also remove the unrelated standardisation of the params column, because when working on the base.sql I saw we have more ugly mix of '' and {}, and it made the diff for this PR hard to read. Shall someone else clean up that mess one day ;-) So now it should be fine. Thanks for your review.

avatar richard67
richard67 - comment - 15 Aug 2020

@wilsonge

We’re still using an empty string for 3rd party extensions. I also ask because the core extensions update scripts will have the empty json string but if they are discover installed from a 3.x update then it will be an empty string. Is it not better to use empty json there too?

I've found a terrible mix of empty string or empty json in the update sqwl scripts, partly even mixed in the same one script. So I won't touch that. For the custom_data I use empty string everywhere. I had also made a standardisation of the "params" to empty json, but that was confusing here so I've reverted it.

Also because you’ve added the property into the base table class I assume we no longer need it in the adapters.

You mean revert all changes in this PR for the "*Adapter.php" files?

avatar richard67
richard67 - comment - 15 Aug 2020

@wilsonge Assuming I understood you right I've reverted the adapter changes. Tested here and it still works. No idea thought why the MySQL system tests fail. Here installation works.

avatar richard67
richard67 - comment - 15 Aug 2020

@wilsonge MySQL system test failure is related. No idea why I don't get that here. Please don't merge this by review as long as this is not fixed.

avatar richard67
richard67 - comment - 15 Aug 2020

Ok, all fixed. Ready for testing.

avatar wilsonge wilsonge - change - 12 Sep 2020
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2020-09-12 00:33:50
Closed_By wilsonge
avatar wilsonge wilsonge - close - 12 Sep 2020
avatar wilsonge wilsonge - merge - 12 Sep 2020
avatar wilsonge
wilsonge - comment - 12 Sep 2020

I want this in beta 4 so merging even though it doesn't have tests. Thankyou @richard67

avatar richard67
richard67 - comment - 12 Sep 2020

@wilsonge Please remember to add a hint in the release notes that people updating from 4.0 Beta 1, 2 or 4 to the Beta 4 have to add the custom_data database column "manually" in their SQL client (e.g. phpMyAdmin). The database fixer won't show anything because there is no "ALTER TABLE ..." statement anymore for this field in the update SQL scripts.

avatar wilsonge
wilsonge - comment - 12 Sep 2020

It will be in the notes

avatar hendrikbehncke
hendrikbehncke - comment - 14 Sep 2020

Hi, @richard67 @wilsonge
thank you guys for handling the issue.
One last change would be super awesome, if the type could be changed from TEXT to LONGTEXT?

avatar wilsonge
wilsonge - comment - 14 Sep 2020

Going to point back to this one I'm afraid #19618 (comment) (not the same field obviously but I'm going to provide the same justification)

Add a Comment

Login with GitHub to post a comment