Problem/Motivation

Currently, it is not possible to add a custom date format, if the same date format string already exists:

Error message: "This format already exists. Enter a unique format string. "

Steps to reproduce:
1. Install Drupal8 Head
2. Go to admin/config/regional/date-time/formats/add
3. Add new date format:
Name: TestFormat
Machine name: testformat
Format String: Y
Languages: English
=> Add format

There should not be a restriction on duplicate custom date formats with identical dates; this feature exists in Drupal 7.

For example: if a user needs a date format for only displaying the year, and then decides to change it later, she or he probably doesn't want to change all fields that display the year, but just the one date format.

Proposed resolution

Remove the restriction to only allow unique date format values.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

#2119903: Show locked date formats in the UI
#2119997: Change UI to remove display machine name for date formats
#2100351: Date format name 'custom' is reserved but not checked for

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because it addresses a regression from Drupal 7.
Issue priority Normal because it doesn't address any errors or major UI problems.
Prioritized changes Bug fixes are allowed to be fixed in the beta phase. See #5 for a real-world example that demonstrates that this isn't just baggage being ported to Drupal 8.
Disruption None.

Comments

ursula’s picture

StatusFileSize
new93.26 KB
new1.02 KB

I attached a proposed solution:

Remove the restriction in core/modules/system/lib/Drupal/system/Form/DateFormatFormBase.php

A problem arose when updating an existing format, but not changing anything:
The patched version would then execute the update, but set a message that nothing has actually been updated:

The existing format/name combination has not been altered.

(also see attached screen shot).

Is a message in this case at all needed?

vijaycs85’s picture

Issue summary: View changes
Status: Active » Needs review

For example: if a user needs a date format for only displaying the year, and then decides to change it later, she or he probably doesn't want to change all fields that display the year, but just the one date format.

IMHO, if the new format is not exist, add one and update the fields those need this new format.

Status: Needs review » Needs work

The last submitted patch, 1: drupal8.datetime-module.2120813-1.patch, failed testing.

malcomio’s picture

Status: Needs work » Closed (won't fix)

I don't see why you would want to create duplicate date formats - they would just clutter up the list.

If the date format a person wants already exists, then they can use that existing format.

If they need to add a different format later, they can do so.

tim.plunkett’s picture

Status: Closed (won't fix) » Needs work

Say you have two different date formats, each with dozens of places referencing them.
Then you are told they should be unified, and you need to make them match.
But you know deep down that the client will change their mind back shortly.

This is not a fictional scenario, this has happened to me.

If you could just make the two date formats match, this would be easy.
But if you had to go through and change every place, and then later change it back, it's a lot more work.

jsbalsera’s picture

Status: Needs work » Needs review
StatusFileSize
new1020 bytes

Rerolled patch.

tim.plunkett’s picture

Issue tags: +Needs tests
mpdonadio’s picture

Status: Needs review » Needs work

Back to Needs Work because this definitely needs tests.

Anonymous’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new1.3 KB
new2.3 KB
new1.3 KB
borisson_’s picture

StatusFileSize
new888 bytes
new2.53 KB

The comment in validateForm was outdated.

The last submitted patch, 10: allow_several_date-2120813-10.test-fail.patch, failed testing.

koence’s picture

Status: Needs review » Reviewed & tested by the community

Tests were added.

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs beta evaluation, +Needs issue summary update
StatusFileSize
new1.3 KB

Attached is also a test-only patch to demonstrate that this works properly. I like how the test checks doesn't check for specific changes in this patch. Manual testing is also OK.

This is RTBC once we update the IS and add a Beta Eval.

mpdonadio’s picture

Anonymous’s picture

The last patch in this issue will be red, so we need to reupload both patches in order for testbot to see the right patch as well.

The last submitted patch, 14: allow_several_date-2120813-test-only.patch, failed testing.

The last submitted patch, 16: allow_several_date-2120813-16.test-only.patch, failed testing.

mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community

I think this is good to go now.

alexpott’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Postponed

I think this should be postponed to 8.1.0 as I can't see how it passes the beta evaluation. The change makes sense and is well implemented but there is no compelling case to make this change in the beta.

mpdonadio’s picture

Version: 8.1.x-dev » 8.0.x-dev
Category: Task » Bug report
Status: Postponed » Reviewed & tested by the community
StatusFileSize
new29.71 KB

Drupal 7 splits out format and type into separate admin forms (admin/config/regional/date-time and admin/config/regional/date-time/formats). The attached image shows where I created the format 'M M M' and assigned it to 'Test 1' and 'Test 2'.

Drupal 8 doesn't allow me to do this, so this is a regression and we should reclassify this as a bug. Updating IS and Beta Eval.

mpdonadio’s picture

Issue summary: View changes

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: allow_several_date-2120813-16.patch, failed testing.

mpdonadio’s picture

StatusFileSize
new261.05 KB

Triggering retests. Don't why this patch affected BlockTest (see attached for the fail).

The last submitted patch, 16: allow_several_date-2120813-16.test-only.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community

Retest of #16 came back green. BlockTest also passed locally. Resetting RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 15b2b5e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation and testing Drupal 7 - nice work.

  • alexpott committed 15b2b5e on 8.0.x
    Issue #2120813 by pjonckiere, mpdonadio, ursula, borisson_, jsbalsera:...

Status: Fixed » Closed (fixed)

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