joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.76k stars 3.64k forks source link

[4.0] null dates ? #29429

Closed brianteeman closed 4 years ago

brianteeman commented 4 years ago

Apologies if I have not checked this in detail myself it is based on forum comments

Assumption 1

When we upgrade from 3.10 to 4 my understanding is that all core tables that have dates with default values of 0000-00-00 are updated to use real null dates

Assumption 2

Any non-core extension with values of 0000-00-00 will have problems on j4 and will need updates from the extension developer.

Request

If my assumptions are both correct is it possible to put a check in the 3.10 update checker that will

  1. Notify the user of any table that will have problems in J4 because of the date
  2. Have a "click here" to update the table.

cc / @alikon @richard67 @zero-24

richard67 commented 4 years ago

@brianteeman What do you mean with

So I am not really sure if we can fulfill these requirements. Since he was involved into decisions related to the null dates changes, especially the update path, I ping @wilsonge here, too. Maybe he has some ideas what we can do.

A (draft) PR to update weblinks for null dates in J4 can be found here: https://github.com/joomla-extensions/weblinks/pull/416. It can server as some kind of a template for extesion developers and also contains info in my comment about what to be clarified regarding the order of updating.

brianteeman commented 4 years ago

It was just some late night thoughts as a result of https://forum.joomla.org/viewtopic.php?f=803&t=980428

richard67 commented 4 years ago

@brianteeman Ah, this one, I know, Anibal asked be some questions in Glip and I answered as well as I could.

In fact we have to think about the migration of extensions, not all has been thought through completely yet, so this issue points on the right thing, even if we might have to do it in a different way.

Independently of this but maybe helpful for this, too, could be to run the schema check after an update, and if errors are found (e.g. due to timed out or for some external reason broken update), point to them and link to the database checker, see the comment here: https://github.com/joomla/joomla-cms/issues/29386#issuecomment-638686803.

anibalsanchez commented 4 years ago

I'm following the question from a forum user about how to handle the data NULL conversion between Joomla 3 and Joomla 4.

According to the Potential backward compatibility issues in Joomla 4.

Strict mode has been enabled. The following flags are now active by default in Joomla 4 and you may have to update your database queries accordingly. This will help us with future MySQL version upgrades and also aligns more closely with Postgres to enable easier compatibility with queries in both languages.

From weblinks/pull/416:

With version 4.0 the CMS will stop to use the deprecated null dates '0000-00-00 00:00:00' on MySQL and stop to abuse '1970-01-01 00:00:00' on PostgreSQL as a pseudo null date. for database columns of type datetime (MySQL) and timestamp without time zone(PostgreSQL). Instead of this, the CMS will use real null values for datetimes/timestamps, except of created and modified times, which will be still not nullable but not having a default value anymore. The modified time and user id will be set to the created time and user id for never modified items so that the behavior is the same like e.g. filesystems do.

These changes were necessary for compatibility with MySQL 8. Not that some functionality has changed in MySQL 8, but the default settings e.g. for strict mode and null date handling have been changed with 8, so problems which already started with 5.7 for some server configurations will happen more likely with 8 (SQL error about invalid datetime values or invalid default value '0000-00-00 00:00:00' for daetime columns).

Then, 0000-00-00 00:00:00 can't be used on Joomla 4 and everything has to be updated to work with NULL dates.

This table is a good overview of what is changing:

  1. The 0000-Null conversion
  2. The Nulled-Mandatory field type conversion, fields that are Nulled on J3 and now they are Mandatory
J3 Table J3 Field name J3 Field type => J4 Table J4 Field name J4 Field type
User registerDate datetime No 0000-00-00 00:00:00 => User registerDate datetime No None
User lastvisitDate datetime No 0000-00-00 00:00:00 => User lastvisitDate datetime Yes NULL
Content created datetime No 0000-00-00 00:00:00 => Content created datetime No None
Content modified datetime No 0000-00-00 00:00:00 => User registerDate modified datetime No None

The questions are:

Bakual commented 4 years ago

I don't think that's something core has to cover. That's the job of 3rd parties to make sure their extensions are compatible with J4 and yes it means adjusting the database tables in this case.

What core needs to do is make sure a real NULL value instead of 0000-00-00 already works in J3.10. Because we promise that an extension can work both in J3.10 and J4.

richard67 commented 4 years ago

@anibalsanchez Ah, it seems I have to update my comment in that PR for weblinks, because we have later decided to alloew null values for the last visit date of users. No, forget it, was misreading something. All ok there.

brianteeman commented 4 years ago

That's the job of 3rd parties to make sure their extensions are compatible with J4 and yes it means adjusting the database tables in this case.

Even if we dont offer to do it for a user on update (and I'm not sure I even agree with doing that upgrade) I strongly believe that we should (if possible) at least warn users. If they are using a custom built extension for example they would never know about this potential issue.

anibalsanchez commented 4 years ago

What core needs to do is make sure a real NULL value instead of 0000-00-00 already works in J3.10. Because we promise that an extension can work both in J3.10 and J4.

Exactly. Then, the upgrade path will be something like this:

brianteeman commented 4 years ago

All the installed extensions on J3 are compatible with J4, and The J4 Upgrade Check on J3 shows that it is ready to go, then

How will the user ever know that. It can only ever be a best effort. If the extension has not been updated to support 4 or it is a custom component it will just say unknown

anibalsanchez commented 4 years ago

@brianteeman Yes, of course, the Upgrade Check can only check the specs.

brianteeman commented 4 years ago

and my point for raising this was if we could programatically check for null dates and at a minimum notify the user

anibalsanchez commented 4 years ago

@brianteeman If Joomla 3.10 is going to support 0000 and NULL dates, then yes, I think that the Update Checker can run a few queries on MySQL structure to find if there is a missing datetime field with 0000 dates.

So, users could check if there is something missing that will not work on J4.

brianteeman commented 4 years ago

exactly

richard67 commented 4 years ago

Yes, but only if we decide to fully change from old fashioned pseudo null dates to real null values on J 3.10 already.

richard67 commented 4 years ago

I need to sleep over that at least one night and fully think through it.

Bakual commented 4 years ago

How will the user ever know that. It can only ever be a best effort. If the extension has not been updated to support 4 or it is a custom component it will just say unknown

I'd say if it says "unknown", you can take it as almost guaranteed that it will not work.

mbabker commented 4 years ago

I would not explicitly check for what Joomla historically treated as null dates as a separate check from extension compatibility. If an extension has reported itself as 4.0 compatible, then it should be safe enough to assume the developer has addressed the null date issue.

mbabker commented 4 years ago

To me, having that as a separate check implies the user is going to have to make database schema changes on their own either after updating the extension to a 4.0 compatible version or after updating their website to 4.0. In a well designed extension update, that will not be the case.

brianteeman commented 4 years ago

this was for extensions that are marked as unknown.

mbabker commented 4 years ago

I still wouldn’t actively suggest it even in that scenario. It’s ultimately leading down a path of telling the user they need to check an extension’s database schema and potentially “core hack” that and the extension code.

The truth of the matter is extensions with 0000 dates should still work OK if the database driver’s getNullDate() method still returns a singular date (off hand I don’t remember how we changed the driver API here) since their queries will be written for that. The breakage comes if they rely on something higher in the core API (something in the root Table class would be a probable issue here) that was changed to use null values.

Basically, I would suggest having that type of check or notification in the UI might cause more problems than it solves unless you are in the technically savvy group.

wilsonge commented 4 years ago

Any non-core extension with values of 0000-00-00 will have problems on j4 and will need updates from the extension developer.

Just to be clear - any existing extension will not have issues using 0000-00-00 (on their current mysql base - it wouldn't have worked on mysql 8 in j3 either) - the new null date code is "hidden" behind this flag https://github.com/joomla/joomla-cms/blob/4.0.0-beta/libraries/src/Table/Table.php#L132 which is set to false by default to keep b/c

Obviously for things integrating with core (and that does include any sort of UCM data I think -need to test that/check the upgrade queries) obviously will get upgraded and therefore anything inserting data into a core table will need to use the new format.

mbabker commented 4 years ago

Actually, let me revise that a little bit.

If they're querying a table that ships as part of the core package, then yeah, there is going to be trouble because 3.x compatible code isn't null value aware.

Anything in the extension's code and database schema will most likely work as long as it's not reliant on something that was changed in the core API (I just looked, the database driver's getNullDate() still returns the 0000-00-00 00:00:00 value so folks are good there), meaning they would most likely run into issues in a model or table class where a feature was adjusted to support null values (which can be fixed by overridding the method in their child class, so it's not the worst issue to work around).

Either way, it's not as simple as "does this table use 0000... dates, if so then it's not 4.0 compatible at all".

wilsonge commented 4 years ago

I think the things most likely to fail are going to be modules/plugins querying data from the content (etc) tables. rather than things doing inserts

bayareajenn commented 4 years ago

I've been struggling with the pre-update check whilst writing the migration from 3.10.x to 4.x documentation for the last few weeks.

From my perspective not knowing anything about 0000s or NULLs but someone who has to write this stuff down so that average users can attempt to migrate from 3.10.x to 4, the pre-update check is a GUIDE. It's a reference point. It's a snapshot. It can't be depended on that if everything is "green" that you're good to do the one-click to 4.

A person still needs to use Extensions -> Manage -> Manage extensively in order to get the full picture. The pre-update check only is pulling extensions that use com_installer (Update Server). It's not going to pull custom extensions or extensions that don't use the update server (and there are many that don't).

I think we need to be careful putting too much stock in the pre-update check component. It's helpful. I love it, don't get me wrong. AND it's a CHECK. It's not the be all, end all. It's not the final word. The final word is when you go through Extensions -> Manage -> Manage and make sure that every single extension installed has been checked that it's compatible with 4.x AND the developer has been checked with to verify the migration path for said extension. Every single one.

I know I'm not ultra dev on anything and I feel a little out of my depth posting on here and yet I've been concerned for weeks now that too much is being put into "trusting" the pre-update check. It's a check. The work still has to be done thoroughly in Extensions -> Manage -> Manage.

I've been figuring out how to word this in the docs so that people understand for if I don't, they'll all have failed migrations. I've had to pause writing the docs for now.

brianteeman commented 4 years ago

Now I am very confused - again.

Is my "assumption 2" correct or not. There are people saying it is and others saying it is not.

If assumption 2 is not correct then everything else related to the "request" is not applicable

anibalsanchez commented 4 years ago

In my opinion, the upgrade paths must work regardless of how the user updates extensions or upgrades Joomla.

  1. J3.10 -> Extensions Updates -> J4 -> Extension Updates
  2. J3.10 -> J4 -> Extension Updates

These upgrade paths imply:

  1. J3.10 has to work with null dates to migrate the 0000 dates before the J4 upgrade
  2. On J4, if an extension is still not updated and there are 0000 dates, then it could complete the extension update to migrate to null dates.

@brianteeman Assumption 2, no 0000 dates on J4. You are right. Joomla 4 is going to have the BD connection in strict mode, so it is not going to accept 0000 dates. They are invalid values.

I think that the main issue is that we don't know:

  1. how an extension update can work with null dates on Joomla 3.10,
  2. how an extension on Joomla 4 can fix the 0000 dates to null dates

It would be great if we could have Weblinks v4 to test both scenarios:

  1. Installing Weblinks v4 on Joomla 3.10, and then upgrade to Joomla 4
  2. Upgrade from Joomla 3.10 (with WebLinks 3.6) to Joomla 4, and then update to WebLinks v4
mbabker commented 4 years ago

Anibal, you aren't helping matters here as you are basically speaking in a tone of "everything will break in 4.0 if your code absolutely requires 0000 dates". That is not true at all.

Anything that interfaces with a database table that is part of the core database model distributed with Joomla must be updated to support null values. There is no getting around that. The core data schema changed, consumers of the schema must update their code to adapt.

Anything that still uses 0000 dates in its own schema will still work just fine. Here is proof, this is running the current 2.0-dev branch of the joomla/database package (so the code included in 4.0) against the users table of a Joomla 3.9 database where I am changing the lastResetTime field.

<?php

require __DIR__ . '/vendor/autoload.php';

$db = (new \Joomla\Database\DatabaseFactory)->getDriver('mysqli', ['database' => 'joomla_cms', 'host' => 'localhost', 'user' => 'my_user', 'password' => 'my_user']);

$db->setQuery(
    $db->getQuery(true)
        ->update('jos_users')
        ->set('lastResetTime = NOW()')
        ->where('id = 597')
);

$db->execute();

$db->setQuery(
    $db->getQuery(true)
        ->select('*')
        ->from('jos_users')
        ->where('id = 597')
);

var_dump($db->loadObject());

$db->setQuery(
    $db->getQuery(true)
        ->update('jos_users')
        ->set('lastResetTime = ' . $db->quote('0000-00-00 00:00:00'))
        ->where('id = 597')
);

$db->execute();

$db->setQuery(
    $db->getQuery(true)
        ->select('*')
        ->from('jos_users')
        ->where('id = 597')
);

var_dump($db->loadObject());

This script runs without errors with the following output.

Michaels-Mac-mini:jfw-database mbabker$ php connector.php 
/Users/mbabker/jfw-database/connector.php:23:
class stdClass#27 (16) {
  public $id =>
  int(597)
  public $name =>
  string(10) "Super User"
  public $username =>
  string(5) "admin"
  public $email =>
  string(20) "admin@localhost.test"
  public $password =>
  string(60) "$2y$10$IyheX5PfY6KHUPj0fzDKB.sPr7XjrQF1lSUh.FWejCSrJVFeLXduS"
  public $block =>
  int(0)
  public $sendEmail =>
  int(1)
  public $registerDate =>
  string(19) "2019-11-08 14:55:54"
  public $lastvisitDate =>
  string(19) "2019-11-08 17:08:43"
  public $activation =>
  string(1) "0"
  public $params =>
  string(0) ""
  public $lastResetTime =>
  string(19) "2020-06-05 11:32:54"
  public $resetCount =>
  int(0)
  public $otpKey =>
  string(0) ""
  public $otep =>
  string(0) ""
  public $requireReset =>
  int(0)
}
/Users/mbabker/jfw-database/connector.php:41:
class stdClass#7 (16) {
  public $id =>
  int(597)
  public $name =>
  string(10) "Super User"
  public $username =>
  string(5) "admin"
  public $email =>
  string(20) "admin@localhost.test"
  public $password =>
  string(60) "$2y$10$IyheX5PfY6KHUPj0fzDKB.sPr7XjrQF1lSUh.FWejCSrJVFeLXduS"
  public $block =>
  int(0)
  public $sendEmail =>
  int(1)
  public $registerDate =>
  string(19) "2019-11-08 14:55:54"
  public $lastvisitDate =>
  string(19) "2019-11-08 17:08:43"
  public $activation =>
  string(1) "0"
  public $params =>
  string(0) ""
  public $lastResetTime =>
  string(19) "0000-00-00 00:00:00"
  public $resetCount =>
  int(0)
  public $otpKey =>
  string(0) ""
  public $otep =>
  string(0) ""
  public $requireReset =>
  int(0)
}

So, the ONLY mandatory changes for extensions are those which are interfacing with a database table that is part of the core database model distributed with Joomla. Extensions are strongly encouraged to apply the same changes made to the core database schema to their extensions for improved MySQL compatibility, but it is not an upgrade blocker.

anibalsanchez commented 4 years ago

Thanks for clarifying @mbabker

alikon commented 4 years ago

sorry to join late to the party we alreday have tags on the manifest info about version etc.... so trust that tag in pre-checker kiss

brianteeman commented 4 years ago

@alikon that is fine for supported extensions. I was hoping to provide some guidance for custom extensions.

To be honest I am now more confused than before as the posts from @anibalsanchez suggested something very different to @mbabker.

Is it therefore correct to say An extension that uses fake null dates will work on J4 if

  1. it is not using those fake null dates with a core table
  2. the mysql server is not 5.7
alikon commented 4 years ago

@brianteeman what supported extensions vs custom extensions where is the difference ? it is the opposite it is an extension custom/ supported or not that need to show the compatibilty tag for j4 or not in update sever manfiest----

...we cannot discover/check all the issues that can come up

alikon commented 4 years ago

ie null date is just an edge case...

alikon commented 4 years ago

btw the abandoned

can help us a bit ....

alikon commented 4 years ago

sadly just discovered that https://github.com/joomla-extensions/weblinks/pull/416 is there pre covid-19 :mask:

bayareajenn commented 4 years ago

Production has on their list to discuss weblinks and com_search in their next meeting.

Bakual commented 4 years ago

@brianteeman To be honest, the nulldate is likely the least of your problems with custom, or not-maintained extensions. There are many other reasons why those extensions may not work in J4. Do you want to test all those cases? Probably not. So why explicitely the null date?

Also, if we add such a check, you would have false positives and false negatives for sure. Because we don't know the code the extension uses.

Best advice for "unknown" extensions is to test with a local backup, which you ideally should do anyway before upgrading. There is no safe way around that.

brianteeman commented 4 years ago

@bakual so far all the components I have written work perfectly apart from one that used tags and needed a few tweeks. The reason for talking about null dates was based on the two assumptions which now appear not to be correct AND its technically easy to change database structures.

I am trying to avoid sticking my head in the sand regarding updates from j3 and not to repeat the mistakes of every previous major update where we ignored updates until it was too late. some of the breaking changes such as in com_content front end views have been known and ignored for over a year already. I was just trying to help!

alikon commented 4 years ago

we all share the same spirit

not to repeat the mistakes of every previous major update where we ignored updates until it was too late.

richard67 commented 4 years ago

We should try to separate the particular issues a bit.

richard67 commented 4 years ago

P.S.: Regarding pre-update check and separate sql for installation and update: What we could do is to check the manifest XML of an extension if it provides sql scripts for J3 and J4. But that might not be necessary for every extension, one might have no datetime columns in database. I have to think about that a bit, and everybody else feel free to do that, too, and suggest ideas.

anibalsanchez commented 4 years ago

@richard67 In my view, the process of updates extensions has to be very simple for the user and the extension developer. The user (or the update manager) has to download a version and install it on the associated Joomla version. The developers have to produce the same extension for Joomla 3.10 and J4.

Following these ideas, com_weblinks and com_search must have installable packages that work for Joomla 3.10 and J4.

Let me know what has to be done to make it happen. I'm not part of the core team, so I know nothing about #29441 I'd be happy to help here or look for contributors to ease the development of extensions.

richard67 commented 4 years ago

Am working on it.

mbabker commented 4 years ago

Why do extensions have to have conditional code to support null values on 4.0 and 0000 dates on 3.x? If it’s their own schema, they should be able to use nulls in 3.x without issue, they just might have to override some methods inherited in the API to make it work. Don’t over-engineer things here.

richard67 commented 4 years ago

@mbabker For their own schema that's true. But for core tables shared with other extensions, like categories it might need different behavior on J3 and J4.

richard67 commented 4 years ago

@mbabker But it could be right that the solution I'm currently thinking about would be over-engineered.

brianteeman commented 4 years ago

Closed due to lack of response - guess no one is interested

zero-24 commented 4 years ago

There has been an PR on that topic i guess? I'm happy to check where the code needs to extended.