User tests: Successful: Unsuccessful:
Pull Request for Issue #19348 .
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.
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.
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.
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.
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):
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.
custom_data
of the extensions
table exists.Result: The column exists.
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.
This test shall verify that that the column custom_data
of the extensions
table is still there after updating a 3.10.
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.
custom_data
of the extensions
table exists.Result: The column exists.
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.
#_
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;
Result: The update suceeds without SQL errors.
custom_data
of the extensions
table still exists.Result: The column still exists.
Result: No SQL errors about column custom_data
not having a default value. The PHP added back with this PR prevents this.
Colum custom_data
of the #__extensions
table is removed when updating 3.10 to 4 and not existing on a new installation of 4.
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.
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 .
Status | New | ⇒ | Pending |
Category | ⇒ | SQL Administration com_admin Postgresql Installation |
I see. Will check later.
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?
Labels |
Added:
?
|
Category | SQL Administration com_admin Postgresql Installation | ⇒ | SQL Administration com_admin Postgresql Installation Libraries |
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?
Title |
|
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.
Title |
|
Category | SQL Administration com_admin Postgresql Installation Libraries | ⇒ | SQL Administration com_admin Postgresql |
Title |
|
Drone failure is unrelated.
Now finally ready for testing.
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.
Keep using empty string. The column may not necessarily be used for JSON.
Insert statements in install SQL need to be updated.
@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.
Title |
|
Title |
|
@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.
@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.
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?
Ok, all fixed. Ready for testing.
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-09-12 00:33:50 |
Closed_By | ⇒ | wilsonge |
I want this in beta 4 so merging even though it doesn't have tests. Thankyou @richard67
@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.
It will be in the notes
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
?
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)
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