Updated as of comment #20

Problem/Motivation

In #2921810: Allow TimestampFormatter to show as a fully cacheable time difference with JS, 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.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

TBD.

Issue fork drupal-3400522

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:

Comments

heddn created an issue. See original summary.

heddn’s picture

Issue summary: View changes

Boobaa made their first commit to this issue’s fork.

boobaa’s picture

Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs issue summary update

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

Also change caused test failures.

boobaa’s picture

Issue summary: View changes
heddn’s picture

Issue tags: +Needs tests

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

agentrickard’s picture

StatusFileSize
new6.28 KB

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

agentrickard’s picture

Status: Needs work » Needs review

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.

If this test fails as I expect, then we can re-commit the patch.

smustgrave’s picture

Status: Needs review » Needs work

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

agentrickard’s picture

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.

agentrickard’s picture

Status: Needs work » Needs review

Here's an updated MR that does the following:

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

agentrickard’s picture

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

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs upgrade path, +Needs title update

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.

anybody’s picture

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

sorlov made their first commit to this issue’s fork.

haydenhuffman’s picture

I will update the title based on the MR and confirm the steps in the updated summary by EOD Monday 2024-07-29.

haydenhuffman’s picture

Title: TimestampFormatter / time_diff missing config schema » time_diff 'description' field not propagated to existing core views, view tests or core schema after being added to Timestamp
Issue tags: -Needs title update

Title updated, removing tag.

haydenhuffman’s picture

Title: time_diff 'description' field not propagated to existing core views, view tests or core schema after being added to Timestamp » time_diff 'description' field not propagated to existing core views, view tests or core schema after being added to TimestampFormatter
haydenhuffman’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Summary updated, removing tag.

sorlov’s picture

Status: Needs work » Needs review

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

need review

smustgrave’s picture

Status: Needs review » Needs work

See upgrade path but still needs upgrade test.

andypost’s picture

andypost’s picture

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)
graber’s picture

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?

graber’s picture

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.

graber’s picture

StatusFileSize
new1.29 KB

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

graber’s picture

Created #3467091: Item (display only) element results in an empty string in form_state values that addresses the root cause. Unfortunately, a few tests fail so it's not going to be that easy.

sorlov’s picture

Status: Needs work » Needs review

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

graber’s picture

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

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

andypost’s picture

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 #3467091: Item (display only) element results in an empty string in form_state values

Previous code was

  // Form structure.
  $types['item'] = array(
    // Forms that show author fields to both anonymous and authenticated users
    // need to dynamically switch between #type 'textfield' and #type 'item' to
    // automatically take over the authenticated user's information. Therefore,
    // we allow #type 'item' to receive input, which is internally assigned by
    // Form API based on the #default_value or #value properties.
    '#input' => TRUE,
andypost’s picture

Issue could be considered a duplicate if hack with input will be moved to contact module in #3467091: Item (display only) element results in an empty string in form_state values

sorlov’s picture

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

emptyvoid’s picture

Just updated code base to 11.2.0 and during database update got a long list of schema errors in almost all of the 50+ views in the system. And promptly both the admin views and interfaces and key site features generate 500 errors or white screens of death.

Warning Notice:
Schema errors for views.view.content with the following errors: views.view.content.display.default.display_options.field-changed.time_diff.description missing schema, views.view.content:display.default.display_options.fields.created.settings.time_diff.description missing schema. These errors mean there is configuration that does not comply with its schema. This is not a fatal error, but it is recommended to fix these issues. For more information on the configuration schemas, check out https:/www.drupal.org/docs/drupal-apis/configuration-api/configuration-schemametadata.

Except, I get 500 errors and white screens of death.. so not trivial.

I'd provide some details based on errors, but this issue white screens the dblog view so...
Reviewing the apache log directly it shows the same information bubbling up from the Drupal framework.

Warnings..
However, manually exporting the config for the view the fields and settings are exported with defaults.

display_options.fields.changed.settings.time_diff.description
display_options.fields.created.settings.time_diff.description
display_options.menu.local_task_custom_parent_route
emilorol’s picture

Applying path #28 solved the problem for me after resaving the field and then the view again without changing any of the two.

Some other warnings too about sequence as well after upgrading core to 11.2.1

tiagoz’s picture

StatusFileSize
new576 bytes

turning #31 MR into patch

tiagoz’s picture

StatusFileSize
new575 bytes
ivnish’s picture

We need to choose one solution here

hitchshock’s picture

My opinion on this matter is as follows:

The patch #28 is more risky and dependent on many things on the site, and it's also a duplicate of #3467091: Item (display only) element results in an empty string in form_state values. We should leave this decision on hold on that ticket until it has been covered by tests and we are sure that it will not cause any new problems.

The solution from #30 just fixes an old issue, so it looks much better for me. And we can be sure that nothing else will be affected.
But, the patch is not ready yet. We need:
- a test that confirms that the issue exists and is now fixed
- upgrade path, to remove 'description' from the existing configs. Why? Because #2921810: Allow TimestampFormatter to show as a fully cacheable time difference with JS was released in 2023, and many configurations already contain this unexpected “description” property

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

herved made their first commit to this issue’s fork.

herved’s picture

StatusFileSize
new842 bytes

Rebased MR 9250, attaching snapshot.