Problem/Motivation

As a module user I want to have a consistent behaviour of date fields.

Steps to reproduce

  1. Add a date field to some content type and limit its granularity to year, month, day
  2. Set this field value via metadata wrapper with something like
    $wrapped_node->{$field_name}->value = $timestamp;
  3. Fetch value from the field

Expected behaviour: fetched value limited to specified granularity.
Current behaviour: fetched value is the same as was set, not changed in any way

Proposed resolution

Apply field granularity to the date in both date_entity_metadata_struct_getter() and date_entity_metadata_struct_setter().

Remaining tasks

  1. Write test - Done
  2. Write patch - Done
  3. Review
  4. Test
  5. Commit

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#21 date-2921146-10-21-interdiff.txt4.79 KBrosk0
#21 date-2921146-21.patch2.99 KBrosk0
#15 Screenshot 2021-04-22 at 5.12.59 PM.png78.1 KBmegha_kundar
#12 2921146-12.patch1.83 KBmegha_kundar
#10 date-2921146-10.patch3.69 KBrosk0
#6 date-2921146-5-6-interdiff.txt1.35 KBrosk0
#6 date-2921146-6.patch3.87 KBrosk0
#5 date-2921146-4-5-interdiff.txt1.46 KBrosk0
#5 date-2921146-5-test-only.patch2.36 KBrosk0
#5 date-2921146-5.patch4.21 KBrosk0
#4 date-2921146-2-4-interdiff.txt715 bytesrosk0
#4 date-2921146-4-test-only.patch2.25 KBrosk0
#2 date-2921146-2-test-only.patch2.38 KBrosk0

Comments

RoSk0 created an issue. See original summary.

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new date-2921146-2-test-only.patch2.38 KB

test only patch.

Status: Needs review » Needs work

The last submitted patch, 2: date-2921146-2-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Status: Needs work » Needs review
StatusFileSize
new date-2921146-4-test-only.patch2.25 KB
new date-2921146-2-4-interdiff.txt715 bytes

Fixed test dependencies.

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Related issues: +#1679552: DateObject::limitGranularity() doesn't work when passed array contains zeroes
StatusFileSize
new date-2921146-5.patch4.21 KB
new date-2921146-5-test-only.patch2.36 KB
new date-2921146-4-5-interdiff.txt1.46 KB

Not sure why new test wasn't run in previous comment.
I restored test dependency declaration, I thought that was changed but according to docs https://www.drupal.org/docs/7/testing/simpletest-testing-tutorial-drupal-7 its not. If I understood it correctly dependency on entity module would be picked up in next 24 hours(https://www.drupal.org/drupalorg/docs/drupal-ci/faq#dependencies).

I fixed expected value in the test - bad TZ.

Patch changes $type value passed to date_entity_metadata_struct_getter() from date_entity_metadata_field_getter() as later only called for fields configured NOT to have end date. In case field is configured to have end date date_entity_metadata_struct_getter() is called directly with type set to "struct" as expected.

Also patch adds another todo to the code base but I don't see any other way of fixing it without waiting for #1679552: DateObject::limitGranularity() doesn't work when passed array contains zeroes to be committed.

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Assigned: rosk0 » Unassigned
StatusFileSize
new date-2921146-6.patch3.87 KB
new date-2921146-5-6-interdiff.txt1.35 KB

Improved patch version using date_granularity().

chris matthews’s picture

chris matthews commented

The year old patch in #6 to date.module applied cleanly to the latest 7.x-2.x-dev. Is anyone able to confirm this issue is ready for RTBC?

Status: Needs review » Needs work

The last submitted patch, 6: date-2921146-6.patch, failed testing. View results

steinmb’s picture

steinmb commented
Issue tags: +Needs reroll
rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new date-2921146-10.patch3.69 KB

Reroll.

steinmb’s picture

steinmb commented
Status: Needs review » Needs work
Issue tags: +Needs reroll 
curl -O https://www.drupal.org/files/issues/2019-03-01/date-2921146-10.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  3781  100  3781    0     0  13360      0 --:--:-- --:--:-- --:--:-- 13360
steinmb@c02dc75xml85 date % git apply -v date-2921146-10.patch
Checking patch date.info...
error: while searching for:
files[] = tests/date_views_pager.test
files[] = tests/date_views_popup.test
files[] = tests/date_form.test

error: patch failed: date.info:21
error: date.info: patch does not apply
Checking patch date.module...
error: while searching for:

  $timezone_db = !empty($item['timezone_db']) ? $item['timezone_db'] : 'UTC';
  $date = new DateObject($value, $timezone_db);
  return !empty($date) ? date_format_date($date, 'custom', 'U') : NULL;
}


error: patch failed: date.module:664
error: date.module: patch does not apply
Checking patch tests/date_entity.test...
megha_kundar’s picture

megha_kundar commented
Version: 7.x-2.x-dev » 7.x-3.x-dev
Status: Needs work » Needs review
StatusFileSize
new 2921146-12.patch1.83 KB
steinmb’s picture

steinmb commented
Version: 7.x-3.x-dev » 7.x-2.x-dev

I think this should stay on the 7.x-2.x-dev branch.

steinmb’s picture

steinmb commented
Status: Needs review » Needs work

Sorry, but the tests are also gone. Did you miss that?

megha_kundar’s picture

megha_kundar commented
StatusFileSize
new Screenshot 2021-04-22 at 5.12.59 PM.png78.1 KB

Hi @steinmb,
I didn't find any test matching for version 7.x-3.x

steinmb’s picture

steinmb commented

Not sure I follow. Look at the latest patch submitted in #10 - https://www.drupal.org/files/issues/2019-03-01/date-2921146-10.patch
is also contain tests for this part of the code making sure it stay fixed.

benjarlett’s picture

benjarlett commented

Thanks for your work, but not quite working for me on 7.x-2.x-dev but maybe it's not for that version

patch -p1 < 2921146-12_0.patch
patching file date.info
Hunk #1 FAILED at 21.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Patches to .info files are always difficult because the version that's in the git repository is modified when you download it as either a packaged release or a dev snapshot. The only change in the info file is to list the new test file, so unless you need to run that test file you shouldn't worry about it.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

I contacted Megha_kundar to see if it might be possible to get that patch file, hopefully it hasn't been lost as it has been a few months.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue tags: +Needs tests

Megha emailed me to say she no longer has that file, so we'll unfortunately need new test coverage.

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll, -Needs tests
StatusFileSize
new date-2921146-21.patch2.99 KB
new date-2921146-10-21-interdiff.txt4.79 KB

Re-roll of #10.

Test migrated from the new file into existing.

Also, we are using patch from #6 in production on a date heavy project since 2018.

Status: Needs review » Needs work

The last submitted patch, 21: date-2921146-21.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.