I'm not sure whether this is a good solution tbh maybe we can apply them via script.php at the upgrade time? Or implement fall backs when that colums does not exists.

I'm not sure whether this is a good solution tbh maybe we can apply them via script.php at the upgrade time? Or implement fall backs when that colums does not exists.

@zero-24 I'm not sure either, but it could be the only way. The stuff in script.php runs as far as I remember after the update sql scripts have been applied, but the error happens before the sql update are applied. A fallback would be executed every time the getTemplate is called during the whole life time of J4, and this just to catch one case during the update. That looks not really reasonable to me.

@wilsonge Any idea(s)?

why we need those new j4 fields in 3.10 install script ?

why we need those new j4 fields in 3.10 install script ?

@alikon Can you read PR descriptions? It says it fixes failed update of J 3.10 to J4, right?

Does our update to J4 make any difference between a newly installed J 3.10 or a J 3.10 updated from a previous version?

Answer is: No, it doesn't.

So that's why we need it in both the installation and the update SQL script.

Well we could try to catch the SQL errror and than fallback to the old query right?

Well we could try to catch the SQL errror and than fallback to the old query right?

Sure we can do that, but as I said, this means we do it for every time this query is tried, during the whole life time of J4. That seemed to be silly to me.

The same applies to doing a "show columns" to check if the columns are there before chosing the right query (with or without these columns).

In the latter case the number of useless checks could be at least reduced by using some static variable somewhere, keeping the result of that check.

If one of the above (exception or check the table structure) is a way to go, and someone provides a PR for J4, I will be happy to close this one here because I also don't like this PR here very much.

But on the other hand I don't want to establish a more silly solution in J4.

Any way it has to be fixed, because currently updating 3.10 to 4 is simply impossible.

3.10 is a combat release for 4.0 it makes sense to do this database change in this version, if we are unable to do it earlier in j4 upgrade process

Well we could try to catch the SQL errror and than fallback to the old query right?

Sure we can do that, but as I said, this means we do it for every time this query is tried, during the whole life time of J4. That seemed to be silly to me.

Well a try/catch block means the catch is only executed when there is a error in the database query. There is no additional code executed on normal sites.

3.10 is a combat release for 4.0 it makes sense to do this database change in this version, if we are unable to do it earlier in j4 upgrade process

Hmm as very last resort maybe but i don't think this is a good solution as this could happen inside future releases too and there we would not have 3.10 or similiar to hide such issue.

Well a try/catch block means the catch is only executed when there is a error in the database query. There is no additional code executed on normal sites.

Ok that's right, except maybe the small "try" part of it.

But an exception might have other reasons, e.g. database server down, and then we would also execute the other fallback query.

Well a try/catch block means the catch is only executed when there is a error in the database query. There is no additional code executed on normal sites.

Ok that's right, except maybe the small "try" part of it.

But an exception might have other reasons, e.g. database server down, and then we would also execute the other fallback query.

Yes and when that seccond query fails too we can still show the error page right?

When the Database server is down we should not reach this part of the code anyway right?

Yes and when that seccond query fails too we can still show the error page right?

When the Database server is down we should not reach this part of the code anyway right?

We never know in which microsecond adb server decides to go down so theoretically we might get to that place of code while the db is up and then try to run the queries while it is down. But that's a very theoretical thing, I must admitt.

trying to catch a query because we failed to update is the wrong approach, if we introduce another column in the template and fail to update then this feature can't be added in the minor version or we find a better solution for this but at this time we have a solution. Every Joomla 4 upgrade has to be on Joomla 3.10 first.

@richard67 thanks for catching this and fixing it. I think the 3.10 is the easier way out here. One question though, as I think this isn't the first time that this is happening, why the order of Joomla update is a. overwrite the files b. apply db migration? I think reversing this order is a more logical procedure (of course it means extracting to a random folder in the /tmp fetching the sqls and then moving the files)

One question though, as I think this isn't the first time that this is happening, why the order of Joomla update is a. overwrite the files b. apply db migration? I think reversing this order is a more logical procedure (of course it means extracting to a random folder in the /tmp fetching the sqls and then moving the files)

@dgrammatiko Thanks for the feedback. I don't really know all the history why the order of processing is like it is, but I remember from the times when we implemented the utf8mb4 conversion that we needed the new, updated database driver to handle the update sql and conversion in the right way, e.g. with a downgrade to utf8 if necessary. So for that the order of processing was good.

Let's wait for more feedback, maybe someone has an idea how to handle it in J4.

Maybe I should open a J4 issue in parallel to this PR so we can track the J4 discussion better?

I mean we are discussing to fix the symptoms here. The true problem is how updating to J4 works. Why does it have to load the menu structure before running the SQL updates? Why do we need to log in some 150 times for that? (ok, it's only 2 times but still annoying). This login thing might be the reason why it loads the admin menu too early. If we would fix that, we would not need any other fix for the issue handled here.

One question though, as I think this isn't the first time that this is happening, why the order of Joomla update is a. overwrite the files b. apply db migration?

It the same if you do update via FTP:
upload files => run DB fix

Doing opposite make no sense, and have more risk to crash the site and corrupt all user data

It the same if you do update via FTP:
upload files => run DB fix

if we don't do it in 3.10 the user can't fix the database with the DB fixer

another "fix" would be change:

->select($db->quoteName(['s.template', 's.params', 's.inheritable', 's.parent']))

to

->select($db->quoteName(['s.*']))

Out of curiosity:

What's the point of routing for the admin (since there's no sef URLs in the backend? (this is what loads the menu)

@Fedik The db fix topic is a different thing: The fixer only runs structural changes, i.e. DDL (data definition language), e.g. CREATE TABLE, ALTER TABLE and so on, but it does not run content changes, i.e. DML (data manipulation language), i.e. INSERT, UPDATE, DELETE. That is the main reason why the old "copy the files and run the db fixer" method makes problems, not the order of processing, if you run the unpacked sql in a temporary folder before you apply the new PHP.

About the routing I have no idea.

About the SELECT * ..: That could work if we later check for each field in the result if it is set.

But it's a dangerous thing because one may make a PR to "optimize" db stuff and replaces the "*" by a dedicated columns list again ;-)

But it's a dangerous thing because one may make a PR to "optimize" db stuff

that true ?

That is the main reason why the old "copy the files and run the db fixer" method makes problems, not the order of processing, if you run the unpacked sql in a temporary folder before you apply the new PHP.

maybe then, the best would be, if the update will be run by separated/isolated application, the same as for installation

User go to /update and run update ?
Also User do upload via FTP, then go to /update, and run update

upd: and for update /update Application, we need /update-update, well, just joking ?

maybe then, the best would be, if the update will be run by separated/isolated application, the same as for installation

You still need to login and check the permissions. Meaning that (at least) session, user and assets are still not covered

If in J4 we would check the database schema version before fetching the menu, we could see the db is still < 4 and could use the old query. Question is: Do we want to read the schema version from database every time?

Guys I think we are wasting time here, please merge this as soon as we have 2 tests for it or come with a solution thats pretty and not a hack.

Well the testing instructions are complete and contain all what is needed ;-)

I have tested this item ✅ successfully on 8ad69db

I have tested this item ✅ successfully on 8ad69db

RTC

Thanks

@richard67 we presumably now need a PR to J4 to remove the column creation scripts

@wilsonge I knew there was something I've forgotten ... will do soon.

@wilsonge Houston, we have a problem: In J4 I have to remove it from the update SQL (or comment it our) in order not to break update from 3.10 now as this PR is merged. But this will break updates from J4 Beta 1 to 3 to the next J4 Beta 4.

Maybe we can go to SELECT * idea, how bad is it? of course leave a comment "danger: do not change because explosion!!!"

Or another idea is to check the version:

...
$query->select($db->quoteName(['s.template', 's.params']));

if ($version >= 4)
{
  $query->select($db->quoteName(['s.inheritable', 's.parent']));
}
...

Both not nice, but may work.

But this will break updates from J4 Beta 1 to 3 to the next J4 Beta 4.

Isn't there a conditional on sql, eg if column doesn't exist create it?

something like

ALTER TABLE `#__template_styles` ADD COLUMN IF NOT EXISTS `inheritable` tinyint(1) NOT NULL DEFAULT 0;

@Fedik let's not add unnecessary conditionals to the runtime if this can be handled in the sql

@dgrammatiko No I think there's nothing like that for alter table add column, but I will check later.

@richard67 I just realised this code is specific to MariaDB ?

PR for removing the SQL for adding the columns from J4 as requested by @wilsonge in his comment above is #30363 . I suggest we continue the discussion in that PR, if necessary (whyt I believe).