-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[4.0] Cleanup previous date time queries and implement new ones for com_content #26295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
27ad9f1 to
91265df
Compare
|
Tomorrow when I’m back to my PC first thing I’ll do is review and test. First look on my mobile phone looks good. |
|
@wilsonge What did you mean with "Has the side effect of this being parseable by the schema checker"? The update statements? Those are DML (data manipulation language part of SQL) and so still not parsed by the database schema checker/fixer. |
|
In table |
|
@twister65 As far as I understand this PR, it is desired that each of these 3 columns |
|
I applied the patch with patchtester, and click on Fix in the system -> database. |
|
"fix" button does only run DDL (data definition language) part of SQL language, i.e. ALTER TABLE, DROP TABLE, CREATE TABLE, but not DML (data manipulation language), i.e. INSERT, UPDATE, DELETE, so the Fix button only changes default values of columns but does not run the update statements for changing values of existing records. One of the secrets of the Fixer 😄 |
|
P.S.: You have to run the update statements of the update sql scripts in PhpPgAdmin. |
|
P.P.S: I.e. DDL changes data structure, DML changes data content. DB checker/fixer can only handle DDL, the DML statements are reported as "xxx statements did not alter structure and so were skipped" (or so). |
|
I also applied this patch in joomla.sql before a new installation with the same result. |
@twister65 To be honest, I did not fully understand your comment anyway. Do you mean the default value of column
And what did you mean with that? What works? I would expect them to be null and have default value of null, too. Maybe you could explain to mea bit more detailled? I really want to be sure I don't understand anything wrong. I could not test this PR myself yet on PostgreSQL. |
|
I mean the values of my new unpublished article created. In the table structure, these three timestamp columns have a default value of NULL. |
|
Sorry, I misunderstand this PR, I thought there was a parser / checker in the code behind for null date values. |
|
|
|
@wilsonge I've just tested new install on MySQL. Com_content columns are like they should regarding definition. When I create a new article in backend, checked out time column has value null, but puplished up and down are still inserted with the '0000-00-00...' datetime value. When I change this to null in PhpMyAdmin, all works fine, i.e. I can set publish up and down, change article status and so on, all fine it seems. So it seems that it needs only to change some code for inserting new articles to get rid of the '0000-00-00...' for the nullable columns (not nullable ones are other thing). Maybe we can take that from @Hackwar 's PR #25760 . SHall I make a PR to your branch, George? |
|
@wilsonge For saving new articles with null value for all nullable columns, i.e. also publish up and down, see wilsonge#38. In addition it also sets the modified column to the created value if modified is null. |
@richard67 Shouldn't modified date be nullable? |
|
@SharkyKZ No, we decided to have it not nullable, and equal to created date for new records. |
|
What's the reasoning behind this? |
|
Was George's idea, maybe for BC reasons or for versioning. |
|
By the way don't we need to update the values in UCM content table? |
|
i think so |
|
@alikon @Quy @SharkyKZ @twister65 and all others: More testers are needed. Instructions you can find in my test result above. I've updated the zip package for update test and the link to it so it is like current 4.0-dev plus this PR. |
|
Thankyou for all you've done to help with on this @richard67 <3 |
|
I have tested this item ✅ successfully on f05c532 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26295. |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26295. |
|
Please commit the 2 suggested changes. |
|
@Quy The ucm_content table is updated for each component in the particular update sql script, see e.g. the other update sql in this PR. |
Co-Authored-By: Quy <[email protected]>
Co-Authored-By: Quy <[email protected]>
|
Thanks @richard67 ! |
|
Thanks @wilsonge .. it was team work. |
1000-01-01 00:00:00. All our values here come from PHP - so they're going to use our null date (always 0000). Has the side effect of this being parseable by the schema checker// cc @alikon @richard67