time_diff 'description' field not propagated to existing core views, view tests or core schema after being added to TimestampFormatter

Created on 9 November 2023, about 1 year ago
Updated 21 August 2024, 3 months ago

Updated as of comment #20

Problem/Motivation

In πŸ› Allow TimestampFormatter to show as a fully cacheable time difference with JS Fixed , a time diff formatting option is provided. Part of that implementation was a "fallback configuration", labeled 'description'. No config schema was added for description, leading to situations where my CI build fails that validates configuration.

Steps to reproduce

Have schema validator enabled; add a timestamp field to a view; try saving the view. Expected: the view gets saved. Actual: a config schema validation exception is thrown.

Proposed resolution

Add schema for 'description' to Drupal core, core views and test views. (the MR does this).

Remaining tasks

Investigate and determine cause of failures mentioned in comment #16 πŸ› TimestampFormatter / time_diff missing config schema Needs work .

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD.

πŸ› Bug report
Status

Needs work

Version

11.0 πŸ”₯

Component
DatetimeΒ  β†’

Last updated 3 days ago

Created by

heddn Nicaragua

Live updates comments and jobs are added and updated live.
  • Needs tests

    The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @heddn
  • First commit to issue fork.
  • Status changed to Needs review about 1 year ago
  • Pipeline finished with Failed
    about 1 year ago
    Total: 2423s
    #53847
  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Can the rest of the issue summary be filled in please.

    Also change caused test failures.

  • heddn Nicaragua

    Obviously we need tests added or this wouldn't have passed any if there were any. Tagging.

  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    Here's a simple view that shows the issue -- let me see if I can work up an MR.

  • Pipeline finished with Canceled
    10 months ago
    #92082
  • Status changed to Needs review 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    OK, here's what I did.

    • Added that view as core/modules/node/tests/modules/node_test_views/test_views/views.view.test_field_created.yml
    • Reverted the schema commit

    If I read the code from core/modules/node/tests/src/Functional/Views/NodeTestBase.php, the setup() method should try to import the new view and then auto-fail schema checks.

  • Pipeline finished with Failed
    10 months ago
    Total: 159s
    #92084
  • Status changed to Needs work 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    There's a test-only feature so don't need to push separately.

  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    Well, I suspect this minor issue may be covering up a more major problem (which we should file separately).

    This is the YML from the test file:

                time_diff:
                  enabled: false
                  future_format: '@interval hence'
                  past_format: '@interval ago'
                  granularity: 2
                  refresh: 60
                  description: ''
    

    This is the YML from core views.view_content

                time_diff:
                  enabled: false
                  future_format: '@interval hence'
                  past_format: '@interval ago'
                  granularity: 2
                  refresh: 60
    

    When the schema change was made in core, the affected core views were not updated. The following core views are potentially affected:

    • views.view.media.yml
    • views.view.test_content_ajax.yml
    • views.view.test_field_created.yml
    • views.view.glossary.yml
    • views.view.content.yml
    • views.view.media_library.yml
    • views.view.media.yml
    • views.view.files.yml
    • views.view.moderated_content.yml
    • views.view.comment.yml
    • views.view.block_content.yml
    • core.entity_view_display.media.type_five.default.yml
    • core.entity_view_display.media.type_two.default.yml
    • core.entity_view_display.media.type_three.default.yml
    • core.entity_view_display.media.type_one.default.yml

    What does this potentially mean?

    1. Core is not updating its views consistently -- for both distribution and tests -- this seems like a process issue
    2. Views only updates yml elements when the plugin usage is added (not edited, so far as I can tell)

    If the affected core views had been updated with the original change was made, tests would have caught the issue.

  • Pipeline finished with Canceled
    10 months ago
    Total: 34s
    #92485
  • Status changed to Needs review 10 months ago
  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    Here's an updated MR that does the following:

    * Removes the new test file
    * Adds the `description` element to `views.view.content.yml`

  • Pipeline finished with Failed
    10 months ago
    Total: 169s
    #92486
  • πŸ‡ΊπŸ‡ΈUnited States agentrickard Georgia (US)

    Update:

    * This merge shows the issue (and the test fails) https://git.drupalcode.org/project/drupal/-/merge_requests/5505/diffs?co...
    * Re-added the original patch / change
    * Updated all affected views

  • Pipeline finished with Failed
    10 months ago
    Total: 241s
    #93168
  • Status changed to Needs work 9 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Seems to have failures

    But if we are updating all views will need an upgrade path for existing sites

    Also not sure the title reflects what the MR is doing so tagged for that.

  • πŸ‡©πŸ‡ͺGermany Anybody Porta Westfalica

    Confirming this issue, we just ran into it. Resaving the view adds the empty string.

  • First commit to issue fork.
  • Pipeline finished with Success
    5 months ago
    Total: 517s
    #206705
  • I will update the title and confirm the steps in the updated summary by EOD Monday 2024-07-29.

  • Pipeline finished with Success
    4 months ago
    Total: 510s
    #237940
  • Status changed to Needs review 4 months ago
  • πŸ‡·πŸ‡ΊRussia sorlov

    Added hook_post_update_NAME() to update existing views with description for time_diff for timestamp formatter

    need review

  • Pipeline finished with Success
    4 months ago
    Total: 485s
    #237987
  • Status changed to Needs work 4 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    See upgrade path but still needs upgrade test.

  • Pipeline finished with Failed
    4 months ago
    Total: 493s
    #239080
  • Pipeline finished with Failed
    4 months ago
    Total: 433s
    #239104
  • πŸ‡«πŸ‡·France andypost

    The test is incomplete but please combine it with checking that field is propagated after upgrade, so we can get rid of "Needs test" tag

        1)
        Drupal\Tests\views\Functional\Update\TimestampFormatterUpdateTest::testViewsFieldPluginConversion
        Exception: Deprecated function: mb_strtolower(): Passing null to parameter
        #1 ($string) of type string is deprecated
        Drupal\Core\Config\Entity\Query\Condition->compile()() (Line: 39)
  • πŸ‡΅πŸ‡±Poland Graber

    Maybe I'm missing something but:
    Drupal\Core\Field\Plugin\Field\FieldFormatter::settingsForm()

        $form['time_diff']['description'] = [
          '#type' => 'item',
          '#title' => $this->t('Fallback configuration'),
          '#description' => $this->t('The configuration below is used as a fallback when JavaScript is not available on the page.'),
          '#states' => $states,
        ];
    

    Description is just a user information element, it doesn't need a schema, how come it got to form state values and exported config in the first place?

  • πŸ‡΅πŸ‡±Poland Graber

    Changing this to:

        $form['time_diff']['description'] = [
          '#markup' => 'The configuration below is used as a fallback when JavaScript is not available on the page.',
        ];
    

    fixes the config after save in views. So the issue is that "item" form element value gets to form state values as an empty string instead of not being passed there at all. On the other hand when saving on entity display settings form, description is not in the config (maybe there's a empty value filter).
    And we need to get rid of those 'description' config values in time_diff everywhere.

  • πŸ‡΅πŸ‡±Poland Graber

    I'd do this. It's a very deep core form API change so let's see if it doesn't break anything.

  • πŸ‡΅πŸ‡±Poland Graber

    Created πŸ› Item (display only) element results in an empty string in form_state values Active that addresses the root cause. Unfortunately, many tests fail so it's not going to be that easy.

  • Pipeline finished with Failed
    3 months ago
    Total: 560s
    #257893
  • Merge request !9250#3400522 Fix timediff empty description β†’ (Open) created by sorlov
  • Status changed to Needs review 3 months ago
  • πŸ‡·πŸ‡ΊRussia sorlov

    Opened separate MR https://git.drupalcode.org/project/drupal/-/merge_requests/9250 for another solution

    Issue was fixed by adding '#input' => FALSE, to $form['time_diff']['description']

    So now we won't have empty description key in formatter config

  • πŸ‡΅πŸ‡±Poland Graber

    Nice one, but shouldn't we fix it in a more global way, adding it to item element defaults instead?

  • Pipeline finished with Success
    3 months ago
    Total: 514s
    #257911
  • Status changed to Needs work 3 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    If a new solution is used will need to update the issue summary, maybe? the title also.

    Would need to check the criteria to see if this change is small enough to warrant not needing test coverage

  • πŸ‡«πŸ‡·France andypost

    New workaround point to problem with item form element inside of forms, git research points to #1825044: Turn contact form submissions into full-blown Contact Message entities, without storage β†’

    So I think if no other option to replace the element with #markup for example the !5505 looks nice idea but surely it needs follow-up to prevent export of this element to config at all, maybe event remove it at form state level via πŸ› Item (display only) element results in an empty string in form_state values Active

  • πŸ‡«πŸ‡·France andypost

    Issue could be considered a duplicate if hack with input will be moved to contact module in πŸ› Item (display only) element results in an empty string in form_state values Active

  • πŸ‡·πŸ‡ΊRussia sorlov

    I'm not sure if this is global issue, related to any item element, cause for example, we have a lot of item elements in FileSystemForm form, but they are not exposed to config values

Production build 0.71.5 2024