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.Follow-up to #2429443: Date format form is unusable
Problem/Motivation
Problem 1: dateFormat() method in \Drupal\Core\Datetime\DateFormatter class does below:
1. Set the requested language as override language.
2. Load the date_format config entity.
3. Save them in $this->dateFormats property for further request.
4. Set the language back to original language.
So the success case (e.g. format 'medium') would have below data in $this->dateFormats property:
Array
(
[medium] => Array
(
[en] => Drupal\Core\Datetime\Entity\DateFormat Object
(
[id] => medium
...
...
)
)
)
However, this is not going to help for 'custom' format - a predefined format for user-defined formats.
Problem 2: The argument $format of formatDate() is very confusing as it is $type in format() method.
Proposed resolution
for
Problem 1: Don't bother doing any of those steps for 'custom'.
Problem 2: Update variable name to use what we have in format()
Remaining tasks
1. Patch
2. Test
User interface changes
N/A
API changes
None.
Beta phase evaluation
| Issue category | Bug because too unneeded processing happens for custom date formats. |
|---|---|
| Issue priority | Normal. |
| Unfrozen changes | None |
| Prioritized changes | Yes, because follow-up on a recent major. |
| Disruption | None |
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | 2457345-exclude-custom-format-load-16.patch | 2.49 KB | vijaycs85 |
Comments
Comment #1
vijaycs85Comment #2
rteijeiro CreditAttribution: rteijeiro commentedLooks good to me!
Comment #3
alexpottLet's add to this condition instead.
Comment #4
alexpottLet's add to this condition instead.
Comment #5
alexpottNot sure that a test is worth it.
Comment #6
vijaycs85Here we go.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedRe #5: not sure how this could be automatically tested. Feedback from #4 was addressed. Back to RTBC.
Added beta eval to summary.
Comment #10
alexpottThis 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 744514b and pushed to 8.0.x. Thanks!
Comment #12
vijaycs85Thank you!
Comment #13
tstoecklerIt might be that I'm once again missing something but I don't think this is correct. The check is for
$format !== 'custom'but in fact it should be$type !== 'custom'because$formatwill be the actual PHP date format, e.g.Y-m-d.Comment #15
alexpott@tstoeckler++ nice catch. Reverted this.
Comment #16
vijaycs85/me feels very bad to make a mistake in a one line fix.
Really sorry about that... It was a blind copy-paste when regenerated the patch at #6.
However, I believe this is just showed how important to name a variable correctly. The type ('medium', 'custom') is getting passed as $type to \Drupal\Core\Datetime\DateFormatter::format and same value is used as $format in \Drupal\Core\Datetime\DateFormatter::dateFormat. Why dateFormat need to accept a '$format' as an argument?
Comment #17
vijaycs85Comment #18
vijaycs85Comment #37
quietone CreditAttribution: quietone at PreviousNext commentedThis appears to have been fixed in #2846643: DateFormatter should not attempt to load a format from storage if the format is "custom" expect for the documentation changes and variable name change to \Drupal\Core\Datetime\DateFormatter::dateFormat.
I am going to close this as outdated and open a new issue for the other changes. Moving credit other there.
Comment #38
quietone CreditAttribution: quietone at PreviousNext commented