joomla-extensions / weblinks

This repo is meant to hold the decoupled com_weblinks component and related code.
GNU General Public License v2.0
45 stars 88 forks source link

[4.0] [WiP] Fix default value for datetime columns and make some nullable to adapt to CMS 4.0-dev and remove sqlsrv #416

Closed richard67 closed 3 years ago

richard67 commented 4 years ago

Pull Request for Issue # .

Summary of Changes

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).

This Pull Request (PR) here implements the necessary changes for the 4.0-dev branch of weblinks so it will be consistent with the behavior of the 4.0 CMS regarding datetimes/timestamps.

In addition it removes the sqlsrv related database scripts because this database type will not be supported in the 4.0 CMS.

It is still work in progress (WiP) because I have to clarify a few things, see my comment below. As long as this is the case it will also have draft status on GitHub.

Testing Instructions

Will be added before removing draft status.

Expected result

Will be added before removing draft status.

Actual result

Will be added before removing draft status.

Documentation Changes Required

I don't think so.

richard67 commented 4 years ago

@wilsonge Do you know how it is planned to do the updateing of the CMS with weblinks installed to 4.0? Shall first the CMS be updated and then the weblinks compontent, or shall it be vice versa? This decides on how datetimes in old data in the categories table for weblinks and corresponding records in the ucm_content table shall be updated. This PR here like it is now will work only on J4, i.e. CMS has to be updated first. On J3 the update statements for the category table in the update sql script will fail.

anibalsanchez commented 4 years ago

@richard67 Back to the idea of having a single codebase for Joomla 3 and Joomla 4, could we have ´isNullDatetime´ backported to Joomla 3.10?

/**
 * Generate a SQL statement to check if column represents a zero or null datetime.
 *
 * Usage:
 * $query->where($query->isNullDatetime('modified_date'));
 *
 * @param   string  $column  A column name.
 *
 * @return  string
 *
 * @since   __DEPLOY_VERSION__
 */
public function isNullDatetime($column)
{
    if (!$this->db instanceof DatabaseInterface)
    {
        throw new \RuntimeException(sprintf('A %s instance is not set to the query object.', DatabaseInterface::class));
    }

    if ($this->nullDatetimeList)
    {
        return "($column IN ("
        . implode(', ', $this->db->quote($this->nullDatetimeList))
        . ") OR $column IS NULL)";
    }

    return "$column IS NULL";
}
richard67 commented 4 years ago

@anibalsanchez I haven't used this function in J4 because as far as I remember here now from scratch, it does the wrong thing. On MySQL it returns also 1000-01-01 00:00:00, which seems having been planned once to be used as some kind of replacement for the old null date, like the 1970 is abused on PostgreSQL. The function uses $this->nullDatetimeList, and this is the one which is wrong.

In J4 we have a few places where we support both, and it could be done similarly here for webslinks.

E.g. for the published up check here in the code changed by this PR it could be done like:

$query->where('(a.publish_up IS NULL OR a.publish_up = ' . $nullDate . ' OR a.publish_up <= ' . $nowDate . ')')

Currently I am a bit out of the discussion because I'm not having the picture what shall be achieved.

anibalsanchez commented 4 years ago

I've been checking how to support 0000 and null dates with the same code on J3 and J4.

These functions are available on J3:

This function is available only on Joomla 4:

So, it looks like they have to be used to avoid writing the queries case by case with literals. Beyond these functions or writing literal conditional queries for each version, it doesn't look like there is a better way in the API to write the queries for J3 and J4.

richard67 commented 4 years ago

@anibalsanchez The nullDate and isNullDatetime functions belong to the database framework https://github.com/joomla-framework/database. The isNullDatetime is the one I've avoided to use because it had the error I've mentioned. Maybe that can be fixed in the framework for J4 (2.0-dev branch of the database package) and the be backported to J3. Beginning on Thursday I might have some time to look into that because we have a long weekend, but today I am tired already. Anyway I think we should ask @wilsonge because he might have a more global idea than I have on how it was thought to be done with extensions.

richard67 commented 4 years ago

P.S.: The getNullDate gets only the old pseudo null dates, it does not get a null value. Furthermore null value support not only depends on the database but on the particular table. That's why the isNullDatetime doesn't help much, it gives only a database wide array with possible null dates (including reall null, but also including the wrong one which I've mentioned. You have to support null values in your table class, and that's what @wilsonge wanted to backport from J4 to J3, therefore his issue https://github.com/joomla/joomla-cms/issues/29441 , and he wanted to clarify some docs writhing for that. I think we should wait for that before we go a wrong way and do useless work for nothing.

anibalsanchez commented 4 years ago

@richard67 It sounds great.

Since the platform is moving aways from date magic values, the removal of ´1000-01-01 00:00:00´ sounds like the right way to make ´isNullDatetime´ usable on J3 and J4.

richard67 commented 4 years ago

@anibalsanchez Yes that would be right, and back integrate it into J3 (and if we work proper also into the master branch of the framework). The only thing I don't know, but maybe @wilsonge or @mbabker can say: Is that $db->getQuery()->isNullDatetime() from the 2.0-dev branch of the framework database package used anywhere, or in general, is that branch of the framework package used anywhere else than in Joomla 4?

For Joomla 4 I can say for sure thet it is not used in the core, not anymore since I've cleaned up, and it's not needed for the core because the J4 core requires real null dates for core tables.

But it might be useful to have it in J4 and in J3 fixed for use by 3rd party extensions.

I'll put it on the to do list for my long weekend to check the situation and what I can do for that.

wilsonge commented 4 years ago

There's definitely some other Joomla projects (e.g. issue tracker) where we're using the framework package. Can't say for sure exactly where we're using that specific function however

richard67 commented 3 years ago

Closing in favour of #436 which includes the changes from this PR here.