[4.0] Setting datetime fields to real null value for com_content #25760

Hackwar · 2019-08-01T18:32:40Z

This hopefully changes all datetime values for com_content from the pseudo-null-date 0000-00-00 00:00:00 to a real null value.

richard67 · 2019-08-01T19:47:38Z

@Hackwar I don't think the database schema checker/fixer understands this in the schema update for MySQL:

ALTER TABLE #__content
MODIFY created DATETIME NULL DEFAULT NULL,
MODIFY modified DATETIME NULL DEFAULT NULL,
MODIFY publish_up DATETIME NULL DEFAULT NULL,
MODIFY publish_down DATETIME NULL DEFAULT NULL;

I think it needs a single line statement for each column for the database schema checker/fixer:

ALTER TABLE #__content MODIFY created DATETIME NULL DEFAULT NULL;
ALTER TABLE #__content MODIFY modified DATETIME NULL DEFAULT NULL;
ALTER TABLE #__content MODIFY publish_up DATETIME NULL DEFAULT NULL;
ALTER TABLE #__content MODIFY publish_down DATETIME NULL DEFAULT NULL;

You could see that here:

https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Schema/ChangeItem/MysqlChangeItem.php#L149-L175

richard67 · 2019-08-01T19:55:32Z

@Hackwar Regarding my comment above: I mean that the database checker will only check the first column modification if you have all in 1 alter table statement. But when the check shows for the first column that the statement has not run yet, it will run the complete statement.

So it is formally not correct to have all in 1 statement.

On the other hand, it might be necessary that we do it like this in MySQL 8, and maybe also lower, if strict mode is on. I noticed on MySQL 8 that when running such statement for a single column in PhpMyAdmin on MySQL8, I get an SQL error complaining about the other column not having a valid default value.

If it turns out we have to do that and so betray the database checker a bit (but it would work), then we have to do that everywhere where this change for real null columns has been made already, because I've seen in schema updates of those PR that they did it like I suggested, with 1 alter table statement for every column.

richard67 · 2019-08-01T20:03:50Z

@Hackwar When running in PhpMyAdmin on MySQL 5.7 with strict mode on, I even have to add the checked_out_time column to make it work, and I have to do all with 1 statement:

ALTER TABLE #__content
MODIFY created DATETIME NULL DEFAULT NULL,
MODIFY modified DATETIME NULL DEFAULT NULL,
MODIFY checked_out_time DATETIME NULL DEFAULT NULL,
MODIFY publish_up DATETIME NULL DEFAULT NULL,
MODIFY publish_down DATETIME NULL DEFAULT NULL;

richard67 · 2019-08-01T20:14:02Z

@Hackwar I've just tested, it needs 1 statement for each column. I will make PR against your repo.

Question is: Don't you want to do the checked_out_time here, too, and close the other PR which does checked_out_time for all?

richard67 · 2019-08-01T20:27:27Z

components/com_content/Helper/AssociationHelper.php


-					$advClause[] = '(c2.publish_up = ' . $nullDate . ' OR c2.publish_up <= ' . $nowDate . ')';
-					$advClause[] = '(c2.publish_down = ' . $nullDate . ' OR c2.publish_down >= ' . $nowDate . ')';
+					$advClause[] = '(ISNULL(c2.publish_up) OR c2.publish_up <= ' . $nowDate . ')';


I think this ISNULL(...) will not work on PostgreSQL @alikon Confirmed?

Meanwhile I've researched and am sure, there is no isnull function in postgresql, it is a mysql special, so you can't use it. I've added the correction to my against your repo.

Do it like this: https://github.com/joomla/joomla-cms/pull/25362/files#diff-65f06a5ca955e1bfe518ccf5bd19bed6R184

@Quy Sure? Does it also work with real NULL values?

@Quy I see, it does work.

@Quy But there are places like the one above where ther is not really used a query object used. Should he do that there, too, and use a query object for that?

Here is the initial PR #21901 that did this way to base off of.

for postgresql there are 2 choices
SELECT (Field IS NULL) FROM ...
or
COALESCE() which is ISO/ANSI SQL standards

This reverts commit 0dab899.

Corrections for MySQL and PostgreSQL for Joomla CMS PR joomla#25760

Hackwar · 2019-08-01T21:25:50Z

I'm going to change the ISNULL to IS NULL and see where the issue in the install SQL is. I would keep the original PR for checked_out and simply provide PRs for everything else and then we can merge everything.

Hackwar · 2019-08-01T21:27:08Z

I will of course do the getNullDateTime() call.

richard67 · 2019-08-01T21:29:04Z

Which issue in the install SQL?

alikon · 2019-08-02T06:16:53Z

some work (for prepared statement) has been done even if in draft wip pr #25179 maybe can help

HLeithner · 2019-08-02T06:29:21Z

libraries/src/Form/Field/CalendarField.php

 			case 'SERVER_UTC':
 				// Convert a date to UTC based on the server timezone.
-				if ($this->value && $this->value != Factory::getDbo()->getNullDate())
+				if ($this->value && strlen($this->value) && $this->value != Factory::getDbo()->getNullDate())


Wouldn't this fit better?
if ($this->value !== NULL && $this->value != Factory::getDbo()->getNullDate())

No, the value could be an empty string, which isn't null.

HLeithner · 2019-08-02T06:29:44Z

libraries/src/Form/Field/CalendarField.php

 				break;
 		}

+		if ($return == '')


Suggested change

if ($return == '')

if ($return === '')

HLeithner · 2019-08-02T06:31:17Z

libraries/src/Table/Content.php


 		// Check the publish down date is not earlier than publish up.
-		if ($this->publish_down < $this->publish_up && $this->publish_down > $this->_db->getNullDate())
+		if (!is_null($this->publish_up) && !is_null($this->publish_up) && $this->publish_up > $this->publish_down)


I think we use === NULL not is_null in code style or?

Suggested change

if (!is_null($this->publish_up) && !is_null($this->publish_up) && $this->publish_up > $this->publish_down)

if ($this->publish_up !== NULL && $this->publish_up !== NULL && $this->publish_up > $this->publish_down)

@HLeithner We use both it seems, but is_null() we use by far more often.

iirc @mbabker changed it in the frame work from is_null to !== NULL I'm and sure if the coding style says anything about this. At least we should use the same in all places.

is_null() is slower. Also null must be in lowercase.

Our codestyle has no rules regarding that. If you want us to refactor to that, then I would ask to make a separate issue for that. You might want to bring that up in the ATT. 😉

Use $query->isNullDatetime()

Quy · 2019-10-14T18:09:55Z

Replaced by #26295.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25760.}

richard67 · 2019-10-14T18:12:25Z

@Quy Same for PR #25715 , will be replaced by #26491 and #26490 .

Setting datetime fields to real null value for com_content

15b6218

joomla-cms-bot added NPM Resource Changed This Pull Request can't be tested by Patchtester PR-4.0-dev labels Aug 1, 2019

One alter table for each column for db fixer

e480596

richard67 mentioned this pull request Aug 1, 2019

Corrections for MySQL and PostgreSQL for Joomla CMS PR #25760 Hackwar/joomla-cms#31

Merged

richard67 reviewed Aug 1, 2019

View reviewed changes

richard67 and others added 3 commits August 1, 2019 22:40

There is no isnull function in pgsql

0dab899

Revert "There is no isnull function in pgsql"

13bda81

This reverts commit 0dab899.

Merge pull request #31 from richard67/patch-1

8d564e0

Corrections for MySQL and PostgreSQL for Joomla CMS PR joomla#25760

HLeithner reviewed Aug 2, 2019

View reviewed changes

richard67 and others added 4 commits August 4, 2019 11:16

Use $query->isNullDatetime

e87bc58

Update QueryHelper.php

d088446

@richard67 facepalm

6c30585

Merge pull request #32 from richard67/4.0-dev-pr-25760-fix-2

71efbbd

Use $query->isNullDatetime()

alikon mentioned this pull request Aug 8, 2019

[4.0] [com_content] [wip] convert to prepared statement #25179

Closed

Hackwar added 2 commits August 15, 2019 11:34

Merge branch '4.0-dev' into j4mysql8content

dd01835

Merge branch '4.0-dev' into j4mysql8content

d1076aa

Quy mentioned this pull request Aug 24, 2019

[4.0] Schedule featured articles #25979

Merged

Hackwar added 2 commits August 25, 2019 09:54

Merge branch '4.0-dev' into j4mysql8content

f8ac7f5

Merge branch '4.0-dev' into j4mysql8content

86387c1

richard67 mentioned this pull request Sep 21, 2019

[4.0] Cleanup previous date time queries and implement new ones for com_content #26295

Merged

This was referenced Sep 21, 2019

Create new content records with null value for published up and down wilsonge/joomla-cms#38

Merged

Further changes for pubish up and down null values wilsonge/joomla-cms#39

Merged

Quy added the Conflicting Files label Oct 1, 2019

ChristineWk mentioned this pull request Oct 14, 2019

Some Notices after Joomla 4.00-alpha12-dev-Development-Update Package #26591

Closed

Quy closed this Oct 14, 2019

Quy removed the Conflicting Files label Oct 14, 2019

Hackwar deleted the j4mysql8content branch October 23, 2020 19:59

	if (!is_null($this->publish_up) && !is_null($this->publish_up) && $this->publish_up > $this->publish_down)
	if ($this->publish_up !== NULL && $this->publish_up !== NULL && $this->publish_up > $this->publish_down)

Uh oh!

[4.0] Setting datetime fields to real null value for com_content #25760

[4.0] Setting datetime fields to real null value for com_content #25760

Uh oh!

Conversation

Hackwar commented Aug 1, 2019

Uh oh!

richard67 commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Aug 1, 2019

Uh oh!

richard67 commented Aug 1, 2019

Uh oh!

richard67 commented Aug 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hackwar commented Aug 1, 2019

Uh oh!

Hackwar commented Aug 1, 2019

Uh oh!

richard67 commented Aug 1, 2019

Uh oh!

alikon commented Aug 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Quy commented Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richard67 commented Oct 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

richard67 commented Aug 1, 2019 •

edited

Loading

Quy commented Oct 14, 2019 •

edited

Loading