[4.0] Remove update SQL for child templates

[richard67]

Pull Request for Issue #30333 (comment).

Summary of Changes

With PR #30192 the child templates functionality has been added to J4. For this also new columns have been added to database table #__template_styles. Unfortunately this broke updating from 3.10 to 4, and so the createion of these new columns has been added to 3.10 with PR #30333.

But this requires to remove the SQl for adding these columns from the J4 update SQL script, because there is no way with pure SQL to add the columns only if they don't exist yet, at least not for MySQL or PostgreSQL (for MariaDB is a way), and trying to add them a 2nd time results in an SQL error.

This PR here does that and so will make updating from 3.10 to 4 work again.

But: It will break updating currently available J4 Beta versions 1 to 3, where the new child templates feature and so the new database columns do not exist, to the next Beta 4.

Therefore this PR is draft and an RFC.

Either we merge this and so break updating of the 4 Betas 1 to 3.

Or we roll back PR #30333 and close this one here and find another way to handle the problem (but I have no idea how).

See PR #30333 for the discussion up to now.

Testing Instructions

Read description of PR #30333 and review code changes of this one here.

Actual result BEFORE applying this Pull Request

SQL error "column already exists" when updating from 3.10, but no SQL error when updating from 4.0 Beta 1 to 3.

Expected result AFTER applying this Pull Request

No SQL error "column already exists" when updating from 3.10, but SQL error about missing columns when updating from 4.0 Beta 1, 2 or 3 to a later Beta which includes this PR.

Documentation Changes Required

If this PR will be merged, it will need a sentence in the release notes of the J4 Beta coming after that, telling that updates of previous Betas are broken.

[brianteeman]

Why comment the lines and not remove them?

[richard67]

Why comment the lines and not remove them?

@brianteeman Because we always did like this in such cases, for documenation purposes.

See here for an old example: https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_admin/sql/updates/mysql/3.4.0-2014-09-16.sql.

I haven't invented that. I was told in past to do so, and so I did from then on.

[brianteeman]

makes no sense to me

[wilsonge]

In this specific case because we're in a new major version it's fine to remove them. Generally speaking commenting out is a touch better (again this is all whilst we're stuck with the current schema checker 🤷 ). I think a sentence in the release notes is good enough personally. Obviously not ideal - but good enough

[richard67]

makes no sense to me

Well, I got used to that meanwhile.

When we had to change or remove SQL statements in old update SQL scripts, we had once decided to do that with a comment so that when somebody compares the content of an update package or an installation package and sees this difference, her or she has an explanation for it.

We are talking about the update sql scripts here, ot about any piece of PHP code where indeed it makes no sense to leave commented out code.

[richard67]

@wilsonge Feel free to do whatever you want with this PR, merge by review or leave open for further discussion.

I am out at this point, I have no energy anymore for the testing instructions.

It sucks if there is somebody who critizizes everything you do without really having an idea what it is about.

[richard67]

(again this is all whilst we're stuck with the current schema checker 🤷 ).

@wilsonge No, this is wrong. This time the schema checker is neither the cause nor the victim of the issue, and it will not stumple over it. This time it is the fact that we call the new code when still having the old database during the update, and the new code reads the admin menu and with it the template styles, whatever that is good for.

And it is the installer which will fail when trying to add the column when they already exist coming from J 3.10, it is not like in past when update was ok but schema checker has shown false errors.

[richard67]

Just for the records, I wish we could undo PR #30333 and close this one here and find a better way to handle the issue. But I see no better way right now, and I've really thought it through a while.

[wilsonge]

Thanks!

[richard67]

@wilsonge Don't forget the release notes when making the next Beta. We broke updating from previous J4 versions with this.