Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Add test coverage for creation date. Make sure that there are no errors when the user create a node with a different "Authored on" than the default or the date or time is invalid.
Proposed resolution
Add test to /core/modules/node/tests/src/Functional/NodeCreationTest.php
Remaining tasks
None
Comment | File | Size | Author |
---|---|---|---|
#76 | interdiff-2031183-58-76.txt | 2.64 KB | martin107 |
#76 | 2031183-76.patch | 3.12 KB | martin107 |
#21 | node-2031183-21.patch | 8.92 KB | tim.plunkett |
#23 | node-authored-date-2031183-23.patch | 9.57 KB | Berdir |
#23 | node-authored-date-2031183-23-interdiff.txt | 6.33 KB | Berdir |
Comments
Comment #1
naxoc CreditAttribution: naxoc commentedI tried reproducing this by creating a node and saving it with a couple of new revisions. The dates looked fine to me, so I wonder if someone can describe the steps to reproduce this?
Comment #2
BerdirIt's possible that it was fixed in the meantime, I'll check.
Comment #3
Cyberwolf CreditAttribution: Cyberwolf commentedI tested this and I see that date/time entered in the "Authored on" text field is not saved. When I edit a node, fill in a correct date in the text field, save the node and edit it again, the date appearing in the text field is something else than I entered. PHP intl extension enabled, Drupal 8 datetime module disabled.
Comment #4
Cyberwolf CreditAttribution: Cyberwolf commentedComment #5
Cyberwolf CreditAttribution: Cyberwolf commentedPatch attached. Needs testing with datetime module enabled, might need work.
Comment #6
Cyberwolf CreditAttribution: Cyberwolf commentedComment #7
catchBumping to critical and making this a beta blocker. Beta blocker on the assumption that we'll need a head2head update to correct the wrong dates if we start supporting that after beta (not sure if we will yet, but I'd rather this blocked that, than forget about it).
Comment #8
BerdirFun.
I wasn't able to reproduce the revision date stuff either, seems to be working quite well now?
I was, however, to reproduce the authored date bugs, quite a few bugs related to that actually. And we have -zero- test coverage for that right now.
So this patch adds test coverage with and without datetime.module. Noticed and fixed the following bugs, some might need to be fixed differently.
- The already pointed out bug that the date field is completely ignored if datetime.module is not enabled
- The form element is actually too small to store the largest possible date string, my example is invalid, but replace with 12 and it should be valid and just as long
- In case of an invalid format, things are weird, and there are array/string notices everywhere and the $date is an array with the date and time keys. Error handling is weird then and so on. Maybe it shouldn't even get that far, I don't know.
That said, not sure if this is a beta blocker, not much worse than any other bug.
Comment #10
BerdirPassed/Failed as expected.
I will change the issue title/summary to be about the authored date unless someone is able to reproduce any bugs related to revision dates. Quite sure that my original bug is resolved, the only thing I can check is look at the test where I originally noticed this and improve test coverage for that as well.
Comment #11
tassoman CreditAttribution: tassoman commentedComment #12
tassoman CreditAttribution: tassoman commentedI'm merging #2145967: Is mandatory Datetime field module while enabling Node module? issue, because of:
Fortunally I have an error also, with backtrace.
Sorry but I'm not able writing unit tests yet :(
Comment #13
tassoman CreditAttribution: tassoman commentedThe problem seems to be:
format_date($node->getCreatedTime(), 'custom', 'Y-m-d H:i:s O')
If you run
date('Y-m-d H:i:s O' , $node->getCreatedTime())
it works flawless.(+EDIT)
I've found a mess on string interpretation! m is the minute, M is the month
, but why?!
More, c is not ISO8601 date. I'm debugging on PHP 5.4, windows :(
After all, I think you should go straight ISO8601 format (2013-12-31T23:59:59+01:00)
Comment #14
tassoman CreditAttribution: tassoman commentedJust tried to reproduce on unix machine (php 5.4), works flawless.
Maybe it's a Windows PHP related issue.
Comment #15
BerdirTests were already written, what this issue needs is manual testing with the patch applied.
Comment #16
BerdirUpdating the title and removing beta blocker tag. Will try to update the issue summary asap.
Comment #17
BerdirUpdated issue summary, who wants to review? :)
Comment #18
jhodgdonI was asked on #2158493: Date error when editing a node, which has been marked as a duplicate, to update this issue with steps to reproduce that problem. So, I've just added this to the summary. I'm not sure that the rest of those issues that were previously listed actually exist, since I didn't see them happening in a clean install today, but I did not remove them, just added a new one with steps to reproduce.
And note that I do not know really if this is a duplicate. It sounds like different issues to me. But I was asked by two people to put this here so here it is. Please don't get mad at me for expanding scope if you think it's a different issue -- just edit the issue summary back and reopen the other issue. Thanks. :)
Given that the tests in the latest patch do not cover this test case, I am marking this "needs work". After I update the issue, I will also try the patch and see if it fixes the problem I identified.
It appears that all the problems listed in the summary have to do with editing nodes, so I also adjusted the title.
Comment #19
jhodgdonI applied the patch on #8. It did allow me to edit the node.
It did not fix all the problems, however. I'm still getting an error message:
Warning: date_format() expects parameter 1 to be DateTime, boolean given in Drupal\node\NodeFormController->form() (line 191 of core/modules/node/lib/Drupal/node/NodeFormController.php).
and I still see the problem illustrated in comment #12 where when you go into Edit, the authored by date/time is garbled.
Comment #20
xjm(Merging "node system" and "node.module" components for 8.x; disregard.)
Comment #21
tim.plunkettCan we get an updated issue summary that includes what remaining test coverage we need?
Rerolled.
Comment #23
BerdirUpdating the tests.
I don't think I've been able to reproduce additional bugs, warnings that show up immediately when editing should be reported in the tests?
Comment #24
jhodgdonRegarding the tests not seeing the same errors as people, part of the reason may be environment. I think on one of the issues that duplicated this one, or a related issue, (several months back) there was a discussion that maybe you had to have the "intl" PECL package installed to see the problem. Although that may or may not be true for the current issues...
On your environment, can you see the problems in the UI if you follow the "steps to reproduce" for the "unable to edit nodes after minimal install profile" case that are in the issue summary?
Comment #25
BerdirWill try again.
Testbots have intl installed, we have a test that fails if you don't.
Comment #26
BerdirOk, I can now verify the problem in the issue summary when php-intl is enabled, I can however not make the tests fail.
The reason seems to be that manually with minimal, it uses intl, in the test, it uses php. I don't know why yet.
Comment #27
catchDifferent php.ini for apache vs. cli?
Comment #28
jhodgdonRegarding #27, if you are running the tests via the Testing page in the UI, that would use the Apache PHP config though, not CLI, right? But that would be something to verify on the bots: that intl is actually set up to be used via the CLI php.ini stuff.
Comment #29
BerdirYes, this wouldn't matter, because the form code runs in apache anyway. And there is a DUBT test I think from #2000384: php-intl module causes problems with Date and Time fields when editing Basic Page, the issue was supposed to fix this problem but it only seems to work for datetime.module.
Comment #30
BerdirFound it.
Intl is only used if there is a country, calendar and langcode. And tests don't have a country.
Attached is a patch that forces a default value for country in tests, just wondering how many test fails that gives us, because the one I tested (MenuNodeTest) completely explodes then.
Comment #32
Gábor HojtsyWoah, that was small country test turned out quite bad :D
Comment #33
jhodgdonWow. Glad you tracked that down! Hopefully all 167 failures there can be fixed by fixing this problem.
Comment #34
marthinal CreditAttribution: marthinal commentedI think I found the problem with the tests.
At Drupal\Component\Datetime\DateTimePlus method format() we have
$format_string_type = isset($settings['format_string_type']) ? $settings['format_string_type'] : static::PHP;
If there's no $settings at this point then when
if ($this->canUseIntl($calendar, $langcode, $country) && $format_string_type == static::INTL)
$format_string_type will be PHP and not INTL.
Let's try adding $settings. Locally the test passes with PHP and INTL (applying #30).
This patch is an attempt for DateTimeFieldTest only.
Comment #36
jibranplease remove.
Comment #37
marthinal CreditAttribution: marthinal commented@jibran I was just debugging, this is not a final patch at all :) Anyway the next time I'll remove that.
So #34 seems to fix one kind of problem with the tests.
The other problem is the warning 'date_format() expects parameter 1 to be DateTime, boolean given'. I think that the problem here is the custom format we are trying to add. And here I have detected that using short or other formats(by default), we have problems because 'MM/dd/yyyy - kk:mm' fails and 'MM/dd/yyyy kk:mm' works. So changing the default format should work. The reason I don't know why(simply the format?) and I have to go sleep now :)
I would like to try a modification for the custom format. This is only to understand a little bit more the bug with the test results.
Comment #39
marthinal CreditAttribution: marthinal commentedI found this bug #2204207: Missing format field when translating a date format using php-intl. while working to fix Config Translation failures.
Comment #40
Berdir#2276183: Date intl support is broken, remove it will remove intl support from core, which should fix the problem that @jhodgdon reported, once that is in we can go back to #23.
Comment #41
catch#2276183: Date intl support is broken, remove it is in.
Comment #42
BerdirYeah it is, so here is an untested re-roll of #23.
This will be simplified further by #2226493: Apply formatters and widgets to Node base fields which might very will solve further bugs discovered here. So waiting for that to land until I do more work on this.
Setting to major as the reason it was critical no longer exists I think. Might even become normal after the mentioned issue lands.
Comment #44
BerdirOk, fixing the test and providing a test-only patch.
Comment #47
BerdirComment #48
yuradoc CreditAttribution: yuradoc commentedI can't reproduce problem. I use minimal profile and made all steps.
I can edit node and date is correctly shows.
I use 5.4.16 with intl enabled.
Comment #49
BerdirCore now longer integrates with intl, so yes, those bugs no longer exist. There are still bugs which are covered by the new tests, that were already there before the discussion switched to intl stuff.
Comment #50
BerdirSo. #2226493: Apply formatters and widgets to Node base fields fixed it all up, can't reproduce any errors anymore and there's only one widget left and datetime.module is not involved.
Here's a re-roll of the test.
Changing this to a normal task for improving test coverage. Issue summary still needs an update.
Comment #52
jhedstromThis still applies. Since the patch is so old though, there's no way to re-trigger a test rerun, so re-uploading the patch from #50.
Comment #58
Krzysztof DomańskiUpdated test. Originally bugs was fixed in the meantime so needs only test.
Comment #59
Kristen PolThanks for the patch. #50 mentions that the original issue has been fixed so that the only thing to do was to add the tests for creation date. I have reviewed the patch and it is clear. I have some minor suggestions below.
Nitpick: When searching for the use of "authored date" or "authoring date" there is only one mention but when searching for "created date" and "creation date" there are many. So, I suggest changing:
"authored dates" to "creation dates"
and
"testAuthoredDate" to "testNodeCreationDates"
Nitpick: Similarly, I suggest changing:
"The Authored on date is invalid."
to
"The creation date is invalid."
Nitpick: Similarly, I suggest changing:
"The Authored on date is invalid."
to
"The creation date is invalid."
Comment #60
Krzysztof Domański@Kristen Pol Thanks for review!
This field has a label "Authored on" added by core/modules/node/src/Entity/Node.php.
We can't change it in this test.
Comment #61
Kristen PolOh, sorry. Right, I was searching and then I got carried away. Please disregard suggestions 2 and 3. Only suggestion 1 is possibly relevant. It's interesting that the label says "authored" when the use of "created"/"creation" is more common. I guess I would lean towards changing those two things in suggestion #1, but it's not super compelling. Marking RTBC.
Comment #62
larowlanCan we get an issue summary update here - it is still referring to issues with Minimal profile etc.
Patch looks good to me too, +1 RTBC
Comment #63
Krzysztof DomańskiComment #65
Krzysztof DomańskiUnrelated test failure.
#3032275: Create a fault-tolerant method for interacting with links and fields in Javascript tests
Comment #67
Krzysztof Domański#2990645: "Build Successful" is treated as a test failure
Comment #69
Krzysztof Domański#2990645: "Build Successful" is treated as a test failure
Comment #71
Krzysztof Domański#2990645: "Build Successful" is treated as a test failure
Comment #73
Krzysztof DomańskiTechnical problem in #72. Back to RTBC.
Comment #75
martin107 CreditAttribution: martin107 as a volunteer commentedWhile looking over this I noticed a little flaw that I want to correct
Its a Drupal policy that tests are in english, unless we are directly testing the string translation system.
we have thousands on cases on t() in the codebase where they are unnecessary but I want a little time while I take the t() calls out of new tests
patch is coming.
Comment #76
martin107 CreditAttribution: martin107 as a volunteer commentedt() removed.
That was already RTBC .. for what is it worth this also looks good to me .. a welcome addition.
Comment #77
Krzysztof DomańskiComment #78
alexpottI've credited everyone who worked on the patch whichever iteration and everyone who made efforts to reproduce the issue and provided feedback on the code.
Comment #79
alexpottCommitted and pushed 3b1eea2af3 to 8.8.x and 98227b9db7 to 8.7.x. Thanks!
As a test only change backported to 8.7.x - I discussed backporting test-only changes with @catch (as a release manager).