Support from Acquia helps fund testing for Drupal Acquia logo

Comments

naxoc’s picture

I 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?

Berdir’s picture

It's possible that it was fixed in the meantime, I'll check.

Cyberwolf’s picture

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

Cyberwolf’s picture

Assigned: Unassigned » Cyberwolf
Cyberwolf’s picture

Status: Active » Needs work
FileSize
1013 bytes

Patch attached. Needs testing with datetime module enabled, might need work.

Cyberwolf’s picture

Assigned: Cyberwolf » Unassigned
catch’s picture

Priority: Major » Critical
Issue summary: View changes
Issue tags: +beta blocker

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

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
6.48 KB
8.92 KB

Fun.

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.

Status: Needs review » Needs work

The last submitted patch, 8: node-authored-date-2031183-8-tests-only.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Passed/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.

tassoman’s picture

FileSize
5.76 KB
tassoman’s picture

"Authored on" field error

I'm merging #2145967: Is mandatory Datetime field module while enabling Node module? issue, because of:

I've started a D8 minimal installation w/o any modules, then I added node module (without any field module), I've added the first node flawless, then trying to editing the node I cannot because of "Authored on" field is passed as Boolean and triggers exceptions about data type.

Then I've added Datetime field module and the error has gone. Now I can't say if Datetime module is required adding Node module, or Node module should manage this exception when Datetime module is disabled.

Fortunally I have an error also, with backtrace.

Warning: date_format() expects parameter 1 to be DateTime, boolean given in Drupal\node\NodeFormController->form() (line 204 of core\modules\node\lib\Drupal\node\NodeFormController.php).

Drupal\node\NodeFormController->form(Array, Array)
Drupal\Core\Entity\EntityFormController->buildForm(Array, Array)
call_user_func_array(Array, Array)
Drupal\Core\Form\FormBuilder->retrieveForm('page_node_edit_form', Array)
Drupal\Core\Form\FormBuilder->buildForm('page_node_edit_form', Array)
Drupal\Core\Controller\HtmlFormController->content(Object, 'node.edit')
Drupal\Core\Entity\HtmlEntityFormController->content(Object, 'node.edit')
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)
drupal_handle_request()

Sorry but I'm not able writing unit tests yet :(

tassoman’s picture

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

tassoman’s picture

Just tried to reproduce on unix machine (php 5.4), works flawless.
Maybe it's a Windows PHP related issue.

Berdir’s picture

Tests were already written, what this issue needs is manual testing with the patch applied.

Berdir’s picture

Title: Regression: Revision dates are wrong when saving a node as a new revision » Node authored by field ignored if datetime.module is not enabled
Issue tags: -beta blocker

Updating the title and removing beta blocker tag. Will try to update the issue summary asap.

Berdir’s picture

Issue summary: View changes

Updated issue summary, who wants to review? :)

jhodgdon’s picture

Title: Node authored by field ignored if datetime.module is not enabled » Problems editing nodes if datetime.module is not enabled
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs tests

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

jhodgdon’s picture

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

xjm’s picture

Component: node.module » node system

(Merging "node system" and "node.module" components for 8.x; disregard.)

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update
FileSize
8.92 KB

Can we get an updated issue summary that includes what remaining test coverage we need?

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 21: node-2031183-21.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.57 KB
6.33 KB

Updating 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?

jhodgdon’s picture

Regarding 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?

Berdir’s picture

Will try again.

Testbots have intl installed, we have a test that fails if you don't.

Berdir’s picture

Ok, 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.

catch’s picture

Different php.ini for apache vs. cli?

jhodgdon’s picture

Regarding #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.

Berdir’s picture

Yes, 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.

Berdir’s picture

Found it.

return $this->intlDateFormatterExists() && !empty($calendar) && !empty($langcode) && !empty($country);

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.

Status: Needs review » Needs work

The last submitted patch, 30: default-country-2031183-30.patch, failed testing.

Gábor Hojtsy’s picture

Woah, that was small country test turned out quite bad :D

jhodgdon’s picture

Wow. Glad you tracked that down! Hopefully all 167 failures there can be fixed by fixing this problem.

marthinal’s picture

Status: Needs work » Needs review
FileSize
3.28 KB

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

Status: Needs review » Needs work

The last submitted patch, 34: 2031183-34-should-fix-datetimefieldtest.patch, failed testing.

jibran’s picture

+++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DateTimeFieldTest.php
@@ -106,17 +106,23 @@ function testDateField() {
+    //debug($format_type);
+

please remove.

marthinal’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

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

Status: Needs review » Needs work

The last submitted patch, 37: 2031183-37.patch, failed testing.

marthinal’s picture

Berdir’s picture

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

catch’s picture

Berdir’s picture

Priority: Critical » Major
Status: Needs work » Needs review
FileSize
9.33 KB

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

Status: Needs review » Needs work

The last submitted patch, 42: node-authored-date-2031183-42.patch, failed testing.

Berdir’s picture

Ok, fixing the test and providing a test-only patch.

The last submitted patch, 44: node-authored-date-2031183-44.patch, failed testing.

The last submitted patch, 44: node-authored-date-2031183-44-test-only.patch, failed testing.

Berdir’s picture

Status: Needs review » Needs work
yuradoc’s picture

After installing with the Minimal install profile, editing nodes is not possible.

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

Berdir’s picture

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

Berdir’s picture

Title: Problems editing nodes if datetime.module is not enabled » Improve test coverage for node authored on widget
Category: Bug report » Task
Priority: Major » Normal
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
3.1 KB

So. #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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Version: 8.1.x-dev » 8.2.x-dev
FileSize
3.1 KB

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Krzysztof Domański’s picture

Updated test. Originally bugs was fixed in the meantime so needs only test.

Kristen Pol’s picture

Status: Needs review » Needs work

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

  1. +++ b/core/modules/node/tests/src/Functional/NodeCreationTest.php
    @@ -154,6 +154,70 @@ public function testUnpublishedNodeCreation() {
    +   * Creates nodes with different authored dates.
    +   */
    +  public function testAuthoredDate() {
    +    $now = \Drupal::time()->getRequestTime();
    

    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"

  2. +++ b/core/modules/node/tests/src/Functional/NodeCreationTest.php
    @@ -154,6 +154,70 @@ public function testUnpublishedNodeCreation() {
    +    $this->drupalPostForm('node/add/page', $edit, t('Save'));
    +    $this->assertSession()->pageTextContains(t('The Authored on date is invalid.'));
    

    Nitpick: Similarly, I suggest changing:

    "The Authored on date is invalid."

    to

    "The creation date is invalid."

  3. +++ b/core/modules/node/tests/src/Functional/NodeCreationTest.php
    @@ -154,6 +154,70 @@ public function testUnpublishedNodeCreation() {
    +    $this->drupalPostForm('node/add/page', $edit, t('Save'));
    +    $this->assertSession()->pageTextContains(t('The Authored on date is invalid.'));
    

    Nitpick: Similarly, I suggest changing:

    "The Authored on date is invalid."

    to

    "The creation date is invalid."

Krzysztof Domański’s picture

Status: Needs work » Needs review

@Kristen Pol Thanks for review!

This field has a label "Authored on" added by core/modules/node/src/Entity/Node.php.

$fields['created'] = BaseFieldDefinition::create('created')
  ->setLabel(t('Authored on'))
  ->setDescription(t('The time that the node was created.'))
  ->setRevisionable(TRUE)
  ->setTranslatable(TRUE)
  ->setDisplayOptions('view', [
    'label' => 'hidden',
    'type' => 'timestamp',
    'weight' => 0,
  ])
  ->setDisplayOptions('form', [
    'type' => 'datetime_timestamp',
    'weight' => 10,
  ])
  ->setDisplayConfigurable('form', TRUE);

We can't change it in this test.

$this->assertSession()->pageTextContains(t('The Authored on date is invalid.'));
Kristen Pol’s picture

Status: Needs review » Reviewed & tested by the community

Oh, 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.

larowlan’s picture

Can 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

Krzysztof Domański’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 2031183-58.patch, failed testing. View results

Krzysztof Domański’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 2031183-58.patch, failed testing. View results

Krzysztof Domański’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 2031183-58.patch, failed testing. View results

Krzysztof Domański’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 2031183-58.patch, failed testing. View results

Krzysztof Domański’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 58: 2031183-58.patch, failed testing. View results

Krzysztof Domański’s picture

Status: Needs work » Reviewed & tested by the community

Technical problem in #72. Back to RTBC.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

martin107’s picture

Assigned: Unassigned » martin107
Status: Reviewed & tested by the community » Needs work

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

martin107’s picture

Assigned: martin107 » Unassigned
Status: Needs work » Needs review
FileSize
3.12 KB
2.64 KB

t() removed.

That was already RTBC .. for what is it worth this also looks good to me .. a welcome addition.

Krzysztof Domański’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

I'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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed 3b1eea2 on 8.8.x
    Issue #2031183 by Berdir, marthinal, martin107, Krzysztof Domański,...

  • alexpott committed 98227b9 on 8.7.x
    Issue #2031183 by Berdir, marthinal, martin107, Krzysztof Domański,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.