Problem/Motivation

#501428: Date and time field type in core included some code to load date formats from configuration, which has been ported around since. It means two config entity loads + cache sets on every cold cache request.

Steps to reproduce

Proposed resolution

Going to see what happens if we remove it.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3587601-nr-bot_l9d476nw.txt98 bytesneeds-review-queue-bot

Issue fork drupal-3587601

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch opened merge request !15610

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review

So we need to add defaults in the element callbacks, but otherwise this works fine. Can see the difference in performance tests.

dcam’s picture

dcam commented
Status: Needs review » Needs work

I left several comments on the MR. There are a couple of optional code style changes. But I saw inconsistency in checking for NULL entity loads. I'm not sure if I'm missing something about the code that makes the checks unnecessary. If I did then I apologize.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Made changes based on some of the feedback and answered the rest I think.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback for this one appears to be addressed

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Needs review

I think this comment https://git.drupalcode.org/project/drupal/-/merge_requests/15610/diffs?f... was not addressed?

Can put back to RTBC if it was.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

From what I can tell they both were covered in https://git.drupalcode.org/project/drupal/-/merge_requests/15610/diffs?c... by moving the variable assignment to the top.

godotislate’s picture

godotislate
he/him
commented

I'm not seeing it? At the top of processDatetime(), we have: $formats = DateFormat::loadMultiple(['html_date', 'html_time']);
But the concern is that the loadMutiple() could have at least one NULL result, and it's not guarded against here. $formats['html_date'] or $formats['html_time'] could be NULL.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Yeah there is one outstanding comment on the MR that is not addressed - note that this is an existing issue.

Note that I think the NULL checks are invalid. Both html_date and html_time are locked formats and cannot be deleted see the system/install config. So I do not think the NULL checks are correct.

catch’s picture

catch
he/him
Primary language English
commented

So the null checks may have been due to the maintenance mode check. But I think we also have test coverage for them. Will try removing first though.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

OK removed the isset() checks, and DatetimeElementFormTest failed.

But we can install system config in that test, then it passes. Given the element is already relying on the config existing in multiple places, let's drop the defensiveness altogether I think.

Note that DatetimeElementFormTest test is not the same as DatetimeFormElementTest which we also have a directory down - could probably consolidate these into one test class in a spin-off issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

+1 DatetimeElementFormTest is just a kernel test that doesn't install all the config it should.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587601-nr-bot_l9d476nw.txt98 bytes

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review
godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Reviewed & tested by the community

Confirmed the unnecessary null checks were removed, and confirm the date formats in question are locked. lgtm.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 941b575 and pushed to main. Thanks!

  • alexpott committed 941b5753 on main 
    perf: #3587601 Avoid loading date formats in element info

By: catch
By...

catch opened merge request !15854

catch’s picture

catch
he/him
Primary language English
commented
Status: Patch (to be ported) » Reviewed & tested by the community

Added a backport MR. Just performance test conflicts so back to RTBC.

godotislate closed merge request !15610

  • godotislate committed 68114871 on 11.x 
    perf: #3587601 Avoid loading date formats in element info

By: catch
By...

godotislate closed merge request !15854

godotislate’s picture

godotislate
he/him
commented
Status: Reviewed & tested by the community » Fixed

Committed 6811487 and pushed to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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