Problem/Motivation

On #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality we are updating all uses of entity-unaware formatters for entity base fields to use formatters that are entity-aware.

This issue is about the date/time base fields (for things like Created time, Updated time, etc. -- base fields on entities that are times and dates).

Proposed resolution

Update the Views data where it uses a 'date' views field handler ( \Drupal\views\Plugin\views\field\Date ) for entity base fields (in the base EntityViewsData class) to use the Field UI formatter instead.

Related issue: #2399211: Support all options from views fields in DateTime formatters updates the Field UI timestamp formatter to include all the options present in Views timestamp formatters.

Remaining tasks

Make a patch, including update path and tests for it.

User interface changes

Not really.

API changes

Entity date fields will use Field UI to format instead of generic Views formatters.

Data model changes

Views on entities that display base timestamp fields would need to have their field handler sections updated (not including ones that display teasers or another view mode). This patch takes care of the Core views that do this.

It also needs an update hook to update other views configs. This hook will go through entity base fields currently set up to use the 'date', 'timestamp', or 'timestamp_ago' Views field handlers, and update them to use the appropriate 'field' handler instead. And this will need a test.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because Drupal is working without this
Issue priority Major because entity fields using Views field handlers instead of Field API handlers are not translatable and have no access control. Dates are important to be translated (month names, days of week, etc.); base timestamp/date fields are not liable to be an access violation issue, so it's not Critical. See parent issue for more discussion.
Prioritized changes Prioritized since it is a bug fix.
Disruption Views in module config/install directories that display base timestamp fields will need to have their config yml files modified, so it's a slight bit disruptive. Would be good to get in before the upgrade path...
CommentFileSizeAuthor
#122 interdiff-118-122.txt4.38 KBmpdonadio
#122 update_entityviewsdata-2455125-122.patch32.78 KBmpdonadio
#118 update_entityviewsdata-2455125-118.patch31.64 KBjhodgdon
#111 update_entityviewsdata-2455125-111.patch31.64 KBmpdonadio
#108 interdiff-102-108.txt24.42 KBmpdonadio
#108 update_entityviewsdata-2455125-108.patch32.11 KBmpdonadio
#102 interdiff-97-102.txt20.57 KBmpdonadio
#102 update_entityviewsdata-2455125-102.patch38.24 KBmpdonadio
#97 interdiff-92-97.txt6.86 KBmpdonadio
#97 update_entityviewsdata-2455125-97.patch19.51 KBmpdonadio
#94 interdiff-92-94.txt4.77 KBmpdonadio
#94 update_entityviewsdata-2455125-94.patch18.31 KBmpdonadio
#92 interdiff-83-92.txt4.17 KBmpdonadio
#92 update_entityviewsdata-2455125-92.patch18.95 KBmpdonadio
#83 interdiff-73-83.txt1.71 KBmpdonadio
#83 update_entityviewsdata-2455125-83.patch14.77 KBmpdonadio
#73 interdiff-71-73.txt903 bytesmpdonadio
#73 update_entityviewsdata-2455125-73.patch14.75 KBmpdonadio
#71 interdiff-62-71.txt18.87 KBmpdonadio
#71 update_entityviewsdata-2455125-71.patch13.87 KBmpdonadio
#62 interdiff-59-62.txt2.18 KBmpdonadio
#62 update_entityviewsdata-2455125-62.patch33.91 KBmpdonadio
#59 interdiff-56-59.txt1.57 KBmpdonadio
#59 update_entityviewsdata-2455125-59.patch33.89 KBmpdonadio
#57 interdiff-55-56.txt2.17 KBmpdonadio
#57 update_entityviewsdata-2455125-56.patch33.78 KBmpdonadio
#55 interdiff-40-55.txt12.94 KBmpdonadio
#55 update_entityviewsdata-2455125-55.patch32.8 KBmpdonadio
#39 interdiff.txt3.59 KBrteijeiro
#39 update_entityviewsdata-2455125-40.patch31.06 KBrteijeiro
#36 update_entityviewsdata-2455125-36.patch31.74 KBmpdonadio
#33 update_entityviewsdata-2455125-33.patch32.81 KBmpdonadio
#31 Screen Shot 2015-04-07 at 7.35.17 PM.png58.21 KBmpdonadio
#31 interdiff-30-31.txt6.89 KBmpdonadio
#31 update_entityviewsdata-2455125-31.patch32.8 KBmpdonadio
#30 interdiff-27-30.txt2.48 KBmpdonadio
#30 update_entityviewsdata-2455125-30.patch26.62 KBmpdonadio
#27 interdiff-22-27.txt10.29 KBmpdonadio
#27 update_entityviewsdata-2455125-27.patch24.77 KBmpdonadio
#25 2455125-WIP.patch24.82 KBmpdonadio
#22 interdiff-20-22.txt3.99 KBmpdonadio
#22 update_entityviewsdata-2455125-22.patch32.43 KBmpdonadio
#20 interdiff-18-20.txt6.9 KBmpdonadio
#20 update_entityviewsdata-2455125-20.patch32.51 KBmpdonadio
#18 interdiff-16-18.txt11.49 KBmpdonadio
#18 update_entityviewsdata-2455125-18.patch27.76 KBmpdonadio
#16 interdiff-13-16.txt7.41 KBmpdonadio
#16 update_entityviewsdata-2455125-16.patch24.9 KBmpdonadio
#13 update_entityviewsdata-2455125-13.patch17.48 KBmpdonadio
#8 update_entityviewsdata-2455125-8.patch8.46 KBmpdonadio
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Issue summary: View changes

Removing one part -- those are just arguments, not field formatters.

jhodgdon’s picture

Priority: Major » Critical

As per parent issue, this issue needs to be elevated to Critical. The base entity fields using Views formatters currently need to be updated to use Entity-aware field formatters instead, to respect entity access.

xjm’s picture

Issue tags: +VDC
iMiksu’s picture

Assigned: Unassigned » iMiksu
iMiksu’s picture

Assigned: iMiksu » Unassigned
mpdonadio’s picture

Status: Active » Postponed

Postponing on #2399211: Support all options from views fields in DateTime formatters. I'll fix this when I am done with that one :)

YesCT’s picture

Issue summary: View changes

.

mpdonadio’s picture

Status: Postponed » Needs review
FileSize
8.46 KB

I'm confused here.

"datetime" fields in views already seem to be using the field formatters.

The "timestamp", "created", "changed" fields are using the views date plugin. Is that what we are talking about? Is this all that is needed? This makes these use the timestamp formatters (TimestampAgoFormatter and TimestampFormatter).

Status: Needs review » Needs work

The last submitted patch, 8: update_entityviewsdata-2455125-8.patch, failed testing.

dawehner’s picture

The "timestamp", "created", "changed" fields are using the views date plugin. Is that what we are talking about? Is this all that is needed? This makes these use the timestamp formatters (TimestampAgoFormatter and TimestampFormatter).

Yeah, we have to switch over in core/modules/views/src/EntityViewsData.php:311 and then convert all the existing views to point to plugin_id: field and the required changes of the field options.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

OK, I'll have a better patch in a day or two.

mpdonadio’s picture

Issue summary: View changes

Unpostponing. They are related, but shouldn't block each other.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
17.48 KB

OK, here is a better starting point. No worth posting an interdiff. The main change is

      case 'timestamp':
      case 'created':
      case 'changed':
        $views_field['field']['id'] = 'field';
        $views_field['argument']['id'] = 'date';
        $views_field['filter']['id'] = 'date';
        $views_field['sort']['id'] = 'date';
        break;

in EntityViewsData, and then a lot of

-          plugin_id: date
+          plugin_id: field

in the YAML for views fields sections (not not argument, filter, or sort).

This will fail w/ a lot of failures/exceptions. Looks like most are related to needing to add options to TimestampFormatter, which should be a straight port (ha!) from the field/Date plugin. I'll work on that next.

Status: Needs review » Needs work

The last submitted patch, 13: update_entityviewsdata-2455125-13.patch, failed testing.

mpdonadio’s picture

And so we all remember, field/Date.php is removed in this patch to make finding usages easier. It will be restored once we have the failures under control.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
24.9 KB
7.41 KB

First pass the a one-to-one mapping of the field/Date plugin to the TimestampFormatter, minus the timeago stuff. Need to figure out how to detect and map those to TimestampAgoFormatter for the views defined currently in YAML. Checked on #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known to see if that formatter is going to be touched.

Status: Needs review » Needs work

The last submitted patch, 16: update_entityviewsdata-2455125-16.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
27.76 KB
11.49 KB

OK, let's see if this set of YAML changes goes better.

Status: Needs review » Needs work

The last submitted patch, 18: update_entityviewsdata-2455125-18.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
32.51 KB
6.9 KB

Status: Needs review » Needs work

The last submitted patch, 20: update_entityviewsdata-2455125-20.patch, failed testing.

mpdonadio’s picture

All of the major work to TimestampFormatter and TimestampAgoFormatter should be done. The formatters should mirror what the field/Date plugin does. This patch should also clean up a lot of the fails and warnings that are likely leading to fails. So, these results should show us where we stand.

I have noticed a really weird bug, though. Not sure if it is with this, or with Views itself.

1. Install a fresh system w/ the patch.
2. Add some nodes
3. Login and goto admin/structure/views/view/content
4. Click on "Content: Changed (Updated)" under Fields
5. Change the formatter to Time Ago
6. Click apply.

On my system, I get back the "An illegal choice has been detected. Please contact the site administrator" message and the settings for for the Default formatter. If you click apply again, it works (look at the live preview). Wuh?

Depending what TestBot says, I may tag this for Core Critical Hours tomorrow, as I may not be able to work on this over the weekend.

mpdonadio’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: update_entityviewsdata-2455125-22.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
24.82 KB

For comparison, this is the same patch w/ the field/Date object back. We need to sort the fails into ones that are b/c the field formatter, and those b/c the field/Date object was missing. Though, use of the field/Date object should really be limited outside of views and views_ui, as we are converting usages. It remaining mainly for contrib (eg, hook_views_data type stuff).

Status: Needs review » Needs work

The last submitted patch, 25: 2455125-WIP.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
24.77 KB
10.29 KB

OK, field/Date.php is in this patch. I think I have all fails fixed except for

  • Drupal\views\Tests\GlossaryTest
  • Drupal\system\Tests\Cache\PageCacheTagsIntegrationTest

Both are failing on cache context problems. GlossaryTest is missing the context for 'timezone', but it is set in the field formatter? PageCacheTagsIntegrationTest is missing the context for 'languages:language_content', but I don't see how/why this patch affected that.

Help please?

Status: Needs review » Needs work

The last submitted patch, 27: update_entityviewsdata-2455125-27.patch, failed testing.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
@@ -31,6 +164,18 @@ class TimestampFormatter extends FormatterBase {
+      if ($custom_date_format == 'r') {
+        $langcode = 'en';
+      }

I guess we should explain this line?

GlossaryTest is missing the context for 'timezone', but it is set in the field formatter?

The help message is a bit confusing. Its missing from the expected contexts, so adding them to the test is perfect, which is exactly what you expect to be there.

PageCacheTagsIntegrationTest is missing the context for 'languages:language_content'

If you look at core/modules/comment/config/optional/views.view.comments_recent.yml you'll see that it uses the following fields: subject and changed.
As of now, subject and changed are both ordinary views handlers, but you change the last one to use a formatter. Given that, it will run through
\Drupal\views\Plugin\views\field\Field::getCacheContexts, which calls out to \Drupal\views\Entity\Render\TranslationLanguageRenderer by default, because of core/modules/views/src/Plugin/views/display/DisplayPluginBase.php:549

Given that just fix the cache contexts and be done with it :)

Well, not entirely done because we need dedicated test coverage for those new formatter settings.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
26.62 KB
2.48 KB

Methinks this will come back green, so I think this can be reviewed.

Will work on test coverage for the settings on TimestampFormatter and TimestampAgoFormatter. Maybe tonight.

mpdonadio’s picture

OK, first pass at a test for the formatter itself. It needs some refactoring and general cleanup.

I think TimestampFormatterTest::testTimestampAgoFormatter() will fail. Inside the test REQUEST_TIME is one value, but inside the formatter it is another value. See screenshot of a `debug(REQUEST_TIME)` for what I mean. This messes up the interval, so differences show up at high granularity. Any ideas?

Second, I am not sure how we should structure TimestampFormatterSettingsTest. Timestamp isn't a valid field type in FieldStorageAddForm::getExistingFieldStorageOptions(), so we can't go through the normal node forms. Is the only option to do it through Views (like views.view.content)?

Status: Needs review » Needs work

The last submitted patch, 31: update_entityviewsdata-2455125-31.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
32.81 KB

Rebased against HEAD. No manual merges. Comments in #31 should still apply.

Status: Needs review » Needs work

The last submitted patch, 33: update_entityviewsdata-2455125-33.patch, failed testing.

dawehner’s picture

+++ b/core/modules/field/src/Tests/Timestamp/TimestampFormatterTest.php
@@ -0,0 +1,211 @@
+class TimestampFormatterTest extends WebTestBase {

What about simply not using a WebTest but instead a KernelTestBase, which then has always the same REQUEST_TIME, as you don't call out to drupalGet() anymore. Does that sound a sane thing for you?

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
31.74 KB

Rebased against HEAD; had to redo change to GlossaryTest. Redid TimestampFormatterTest as a KernelTestBase. Lot cleaner. Think this will come up green, unless the rebase broke something.

alexpott’s picture

Priority: Critical » Major

Discussed with @effulgentsia, @webchick and @xjm - none of us could come update with a reason why disclosure of a base field timestamp on a content entity type provided by core would be a critical. If a site did implement a custom entity which had timestamp base field that needs field access rules it could implement it's own views integration to do this.

Leaving at major because I think that doing this before the D8 release would bring consistency to the entity views integration.

dawehner’s picture

Mh, I don't really get that policy ... yes maybe disclosing the timestamp isn't a problem but security wasn't the only reason for those issues.
Its also about multilingual support.

rteijeiro’s picture

Re-rolled and fixed a few nitpicks.

jhodgdon’s picture

So... I was just looking at the options on this formatter... I don't really understand from the UI what they mean. How is someone supposed to know what "Time ago" means, vs. "Time span"? It looks to me as though the only difference is that one has +/- in front, and the other has "ago"/"hence" after it.

I also don't understand why anyone would want to use "Time ago"/"Time hence" (where they would have to know for sure whether the times were in the past or future) instead of "Time span (with "ago/hence" appended)"?

I guess it just seems very complicated to have those options and I don't think they're really clear in the UI. In the code, they're all using the same formatting, except for some prefixes for future/past times, aren't they? And no one would really want a result that was something like "-5 years ago", if they happened to choose "time ago" but the time was actually in the future? What happens with a "raw" time ago if they got the polarity wrong anyway?

So... could this be simplified, since we're basically starting from scratch here anyway and defining a new formatter for time/date fields -- why not do it in a way that has a good UI from the beginning? So you could just have options for "Prefix for future", "Prefix for past", "Suffix for future", "Suffix for past", "Granularity", and wouldn't that take care of everything? Or am I missing something?

mpdonadio’s picture

I agree that this is overly complicated. Personally, I don't think this needs any options. If the interval is positive, use the 'raw time ago' format, if it is negative, use essentially the same format but change the wording a bit. I just don't see real use cases outside of these two.

However, this is a straight port of the views formatter to a field formatter, so I kept everything the same as changing the options will complicate the D7 to D8 migration path if anyone happened to use the other options.

Views maintainers, what say you?

jhodgdon’s picture

Hm. Ok, let's think about this. Really, "hence" is totally weird to me -- this is not something I would ever really use in English...

So if I were formatting a date that could be past or future, and I wanted to use a relative time, I would probably want to format it as:

1 year 3 days ago
1 year 3 days from now

Someone else might have a different preference, such as "ago" and "hence", or maybe they'd want to say
past - 1 year 3 days
future - 1 year 3 days
who knows?

So I think the best thing is really to have 4 text boxes, for the prefix/suffix for past/future. This would be flexible enough for anyone, since they could leave them blank, put in a - or +, or whatever text they wanted.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

Unassigning myself. My availability the next two or three weeks may be spotty.

mpdonadio’s picture

Status: Needs review » Needs work

OK, had brief IRC conversation with @dawehner and he is for simplifying the timeago stuff. We should move forward with the idea from #44 to use the prefix/suffix fields when a user want timeago. Since https://www.drupal.org/node/2456521 is frozen, we aren't going to use the new method that it introduces; that can be done as a followup.

If anyone wants to work on this feel free; I'll assign myself if I can actually block out some time for it.

mpdonadio’s picture

Status: Needs work » Postponed

I am postponing this on #2399211: Support all options from views fields in DateTime formatters so we can have the same UX with both datetime and timestamp fields.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Postponed » Needs work

Since #2399211: Support all options from views fields in DateTime formatters is pretty much wrapped up, and everyone seems fine with the general approach with the timeago fomatter, I am going to get going on this again.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

Unassigning myself as I have an issue with my MAMP, and someone may be able to work on this at the con.

jhodgdon’s picture

@mpdonadio: Are you planning/able to get back to this one?

jhodgdon’s picture

One thought on the last idea: I think that prefix/suffix fields are not great, because in some languages it would be prefix and in others it is a suffix, making it difficult to do string translation. For instance, in English we might say "1 month ago", which you might put as "suffix: ago". But in Spanish, you'd say "hace 1 mes", which would be "prefix: hace". So you can't translate the "ago" suffix string to Spanish, because it has to move to being a prefix string.

So the string you want to translate for t() should be something like "%time ago" to allow it to translate into Spanish as "hace %time".

I am not sure how that fits with views field config, but I don't think prefix/suffix is really the way to go, because on localize.d.o the individual strings in the config will be translated separately and this will not work.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Yes; finally rebuilt my MAMP at home. Up next on my list. Was planning on same approach as #2399211: Support all options from views fields in DateTime formatters which doesn't really do prefix or suffix but past and future placeholder strings. I think that should handle the translation issues.

jhodgdon’s picture

Great, thanks!!!

mpdonadio’s picture

I'm not going to officially postpone this, as I think we will quickly come to a quick solution, but I am going to hold off work on this until the timezone appending brought up in #2399211-118: Support all options from views fields in DateTime formatters gets ironed out.

mpdonadio’s picture

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Postponed » Needs review
FileSize
32.8 KB
12.94 KB

I think #2399211: Support all options from views fields in DateTime formatters is close enough that I can unpostpone this.

I think this captures everything from that issue in this timeago formatter. I did not update TimestampFormatterTest yet, but I want to see if any other tests fail.

Status: Needs review » Needs work

The last submitted patch, 55: update_entityviewsdata-2455125-55.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
33.78 KB
2.17 KB

Fixing up TimestampFormatterTest was easier than I thought.

Status: Needs review » Needs work

The last submitted patch, 57: update_entityviewsdata-2455125-56.patch, failed testing.

mpdonadio’s picture

I think this will fix the schema problems, and I spot checked a few of the failures. Not sure if anything else needs to be addressed yet.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

Unassigning myself. #59 is green, and I think this is ready for review.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -23,7 +29,134 @@
    +
    +    $summary[] = $this->t('Date format: @date_format', array('@date_format' => $this->getSetting('date_format')));
    +    $summary[] = $this->t('Custom date format: @custom_date_format', array('@custom_date_format' => $this->getSetting('custom_date_format')));
    +    $summary[] = $this->t('Time zone: @timezone', array('@timezone' => $this->getSetting('timezone')));
    

    So by default we show 'Time zone: @timezone, what about checking if the timezone if something else than the empty string? Similar to the custom date format, its often not set

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -31,6 +164,20 @@ class TimestampFormatter extends FormatterBase {
    +    $timezone = $this->getSetting('timezone') != '' ? $this->getSetting('timezone') : NULL;
    

    What about using $this->getSetting('timezone') ?: NULL;

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -31,6 +164,20 @@ class TimestampFormatter extends FormatterBase {
    +    if ($date_format == 'custom') {
    

    Let's use ====

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
    @@ -31,6 +164,20 @@ class TimestampFormatter extends FormatterBase {
    +    if ($date_format == 'custom') {
    +      $custom_date_format = $this->getSetting('custom_date_format');
    +      // If an RFC2822 date format is requested, then the month and day have to
    +      // be in English. @see http://www.faqs.org/rfcs/rfc2822.html
    +      if ($custom_date_format == 'r') {
    

    The entire conditions could be one line, and so probably easier to read

mpdonadio’s picture

jhodgdon’s picture

Status: Needs review » Postponed
+++ b/core/config/schema/core.entity.schema.yml
--- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php

Um... Why is this patch updating TimestampAgoFormatter and TimestampFormatter etc.? I thought that was supposed to be taken care of in #2399211: Support all options from views fields in DateTime formatters. while this issue would be just "Use the Field API formatter in Views"?

I don't get it. But maybe you just included that other patch here to see if the tests would pass? Anyway if that is the strategy, we should just postpone this issue again until that other one is *committed* instead of mixing the two patches together. It's too hard to review this way.

And if there are additional changes needed in the base Field formatters to accommodate this issue, I think we should do them on the other issue, right?

I'll go ahead and postpone this; correct status if I'm wrong about this.

jhodgdon’s picture

Status: Postponed » Needs review

I sorted this out on #2399211-155: Support all options from views fields in DateTime formatters. The other issue is for the Date module's added-field formatters, and is limited to that. This issue covers the timestamp formatters used for base fields, and makes Views use those for base timestamp fields too. So the two issues are separate after all.

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Postponed

No, I take it back again. The other issue was always supposed to cover fixing the timestamp base field formatters, but it didn't. I set that back to needs work and this one still needs to be postponed. We need to do the timestamp formatters over there not here.

Slight clarification in the issue summary here as well.

mpdonadio’s picture

So if we are going to do the timestamps as part of #2399211: Support all options from views fields in DateTime formatters, then can we close this as a dup? I'm not sure if there will be anything left in this patch, or where to divorce out just the formatters? I'll look at it closer tonight.

jhodgdon’s picture

Well, as I see it, the other issue is "Make sure that the Field formatters from DateTime module and the ones used by the base entity timestamp fields have all the options". This issue is "Make sure that Views uses Field UI formatters for timestamp base fields on entities, not its own field handlers".

So this issue would still have the changes to the base EntityViewsData class, and associated changes to views that display the time updated/created/etc. fields for entities. Those are not part of the other issue. Right?

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Just assigning this to myself. I reviewed the lastest patch, and I have a plan of attack for moving some of this into the other formatter issue, which would just leave the view-only changes in this one. Stay tuned.

jhodgdon’s picture

Status: Postponed » Needs work

We can un-postpone this now!!!!!!!!!

mpdonadio’s picture

Yup. I'll have a revised version in a day or two w/ the Timestamp formatters stripped out.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
13.87 KB
18.87 KB

This is our new starting point. This is the patch against HEAD, minus the cherry picks that ended up in #2399211: Support all options from views fields in DateTime formatters. The attached interdiff was generated with the interdiff utility, and not a `git diff` between branches. All it really shows is that it reverts changes to four files.

No clue if this will pass, and I still need to do a pass to see if there are any new views that are using the date plugin that will need to be converted.

mpdonadio’s picture

Status: Needs review » Needs work

Some new views landed that use the date plugin. Need to do some side testing to find all of them.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
14.75 KB
903 bytes

I think this catches all of the views. The only date plugins I am finding in views are sorts and filters. Someone else should eyeball this, though, to be sure.

The statistics module has the timestamp column as an int, which means it is a timestamp (as opposed to a date). But, statistics_views_data() exposes it using the date plugins. Since statistics aren't entities, we can't covert this case, correct?

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Right. We're only concerned about entities, and in the exhaustive survey on the parent issue, the only uses I found in entities for the timestamp Views field handlers was in the base EntityViewsData class itself. And we do not have to worry about filters or contextual filters / arguments -- just field handlers.

So, this looks perfect to me! The code and YML changes are all correct, and they convert from using the Views date/time generic formatters to using Field API formatters.

jhodgdon’s picture

Issue summary: View changes
Issue tags: +D8 upgrade path

Beta eval... Also since .yml files for views need updating, I guess this is Upgrade Path material? Would be good to get committed soon to avoid that.

jhodgdon’s picture

Issue summary: View changes

Adding notes about the data model changes.

jhodgdon’s picture

Issue tags: +D8MI
jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Well, this didn't get in before July 1st, so I guess we need to do something about the update path?

jhodgdon’s picture

Issue tags: +Needs upgrade path

I guess there's a tag.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs upgrade path

I talked to Gabor in IRC just now and we think we do not need an upgrade path for this. Hopefully. Explaining in summary.

alexpott’s picture

However, existing views should work OK without being updated, because Views actually doesn't use the handlers in the views config, it uses the ones in the EntityViewsData or hook_views_data() when it comes time to render its fields. So, even though this is a data model change, it does not require an upgrade path.

But won't this break the expected behaviour of save? Create view, apply this patch, load and save the view through API without anything changing then the view will have unexpected changes?

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs work

OK, I did a fresh install of HEAD and exported the user_admin_people view: user_admin_people_HEAD.txt

I applied the patch from #73, did a `drush cr` and exported the view: user_admin_people_PATCH1.txt

I then changed the number of results from 50 to 49 and saved, and exported the view: user_admin_people_PATCH2.txt

You will see the the view config is OK for the formatters (there is a one line diff between user_admin_people_PATCH1.txt and user_admin_people_PATCH2.txt)

However, when I apply the patch, the created / last access columns fall back to the default timestamp formatters when I visit admin/people or the preview

I also noticed that late in #2399211: Support all options from views fields in DateTime formatters, @time got changed to @interval, so this patch needs to be updated for that, anyway. If I do that, and do a fresh install w/ the patch, everything is as expected.

So, at the very least, we need to s/@time/@interval/ in the patch. And, then I assume we need upgrade hooks and tests for all of the views we changed?

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
14.77 KB
1.71 KB

Just fixes the @time => @interval change in a few views. Will think about the rest later...

dawehner’s picture

Status: Needs review » Needs work

So this is actually needs work?

mpdonadio’s picture

Yeah, forgot to come back to this after TestBot got done. Looks like we need upgrade hooks and upgrade tests now since we missed the 7/1 deadline.

jhodgdon’s picture

Do we even know how to do upgrade hooks and tests for this type of change?

mpdonadio’s picture

If 'we == @mpdonadio', then not really. I suspect we need to load each view that is affected, update the settings for the field(s) that changed, and then save the view. I am not 100% sure what the test would do, other than load the view and check the settings?

dawehner’s picture

We should iterate over the config objects, note, update functions should avoid triggering hooks etc., see #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager)

In there we should do the mapping between the previous and now updated configuration.
http://cgit.drupalcode.org/head2head/tree/head2head.module#n309 had such an example for most of the other conversions, so I think this should be doable to achieve.

Gábor Hojtsy’s picture

@mpdonadio: want to take a stab at this? :)

mpdonadio’s picture

In case I'm not the one that does this (...), then one other thing to keep in mind is that we don't want to update all fields whose plugin_id==date, only those attached to entities, and I suppose the update hook would also have to chase this through relationships and not just rely on the base table.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

/grumble

Likely will start this over the weekend...

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
18.95 KB
4.17 KB

I think this works, but someone else should manually test it. It's also a little rough. Didn't do the tests yet.

Status: Needs review » Needs work

The last submitted patch, 92: update_entityviewsdata-2455125-92.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
18.31 KB
4.77 KB

This should make the current patch come up green, but still haven't wrapped my brain around the update tests yet. Any review of the update hook appreciated, so this has a chance of landing before the next beta.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Issue tags: +beta target

Spent more time with this. I don't really know what / how to test this. Halps!

jhodgdon’s picture

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

Looking good! I am not sure about the tests either, but do the issues dawehner referenced above have tests?

  1. +++ b/core/modules/views/src/EntityViewsData.php
    @@ -353,7 +353,7 @@ protected function mapSingleFieldViewsData($table, $field_name, $field_type, $co
           case 'changed':
    -        $views_field['field']['id'] = 'date';
    +        $views_field['field']['id'] = 'field';
             $views_field['argument']['id'] = 'date';
    

    Does this need to have a 'type' added to it too? I'm just looking at all of the views configs and they all seem to have type: timestamp added to them... but maybe that is just the default so I guess we don't do it in the views data.

    So, ignore this. :)

  2. +++ b/core/modules/views/views.install
    @@ -11,3 +11,89 @@
    +    // @todo
    +    // - only operate on field on entities
    +    // - handle timeago
    +
    

    I guess that in addition to the tests, these @todo items need to be addressed too. :( No idea how we would do this. Can of worms. Too bad we didn't get this in sooner. :((((((

    But actually, it looks like you handled the "ago" part below in the code.

    So all it needs is a check to verify that it's an entity field.

    So looking at a typical entity view config, like views.view.content.yml in the Node module, it looks like the fied config has lines like:

    entity_type: node
    entity_field: changed

    in it. So I think you can just do something like:

    if (isset($field['entity_type'] && $field['plugin_id' === 'date')

    and the entity part will be taken care of.

  3. We also need to update the issue summary, because the "data model" section says we don't need an update hook and apparently we do.
mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
19.51 KB
6.86 KB

#96-1, this just follows the pattern of the other field types, and the 'type' key is never set in EntityViewsData::mapSingleFieldViewsData().

#96-2, Good idea. Not sure if we can do better.

This includes a few other small things, and starting to stub out the update test. Still no real test, and keep getting distracted trying to document the data model change.

jhodgdon’s picture

mpdonadio and I were discussion how to write a test for this issue in IRC today. So that these ideas do not get lost, here is a transcript (slightly edited):

(03:09:30 PM) jhodgdon: mpdonadio: that issue that dawehner referenced has a patch with a test: https://www.drupal.org/files/issues/2528178-26.patch
(03:11:19 PM) jhodgdon: mpdonadio: looks like you have to (a) define a database thing to set up the previous database structure, and then (b) write a test that verifies it is updated corretly.

(03:11:53 PM) jhodgdon: mpdonadio: I was looking at the modules/system/tests/fixtures/update/drupal-8.block-context-manager-2354889.php file in the patch
(03:12:14 PM) jhodgdon: mpdonadio: it looks like that is setting up some block configs; in ours we would need to set up a view config
(03:12:38 PM) jhodgdon: probably you could use one of the Core views that the patch you already wrote is updating, with a different machine name?
(03:13:21 PM) jhodgdon: mpdonadio: so the first part of that file just defines 3 block config arrays. I think we could do with 1 view config as an array probably?
(03:13:34 PM) jhodgdon: mpdonadio: then very bottom of that file says +foreach ($block_configs as $block_config) {
+ $connection->insert('config')
(03:14:07 PM) jhodgdon: that is where those block configs are getting added to the database. we'd need to add it to the 'config' table as name 'view.something' I guess
(03:14:22 PM) mpdonadio: jhodgdon: think i need a custom one to test the three updates: timestamp, timestamp ago, and raw timestamp ago ...
(03:14:37 PM) jhodgdon: mpdonadio: but that could all be in one view config I think
(03:14:54 PM) jhodgdon: with 3 fields, or 3 different versions of one created field or something like that
(03:15:04 PM) jhodgdon: or one view with three displays or something
(03:15:13 PM) jhodgdon: or 3 views if that is easier.

(03:15:37 PM) jhodgdon: mpdonadio: so then the class BlockContextMappingUpdateTest extends UpdatePathTestBase
(03:15:58 PM) jhodgdon: it looks like you'd need the 'bare.standard' database dump, as well as your custom one with the views configs in it.
(03:16:05 PM) jhodgdon: in the setUp() function
(03:16:37 PM) jhodgdon: and then in testUpdateHookN() it looks like you do runUpdates(), then do your asserts
(03:17:21 PM) jhodgdon: mpdonadio: and I think what you'd want to do in that test is not what the Block test is doing, but basically just do a $config->get('your view config ID from the dump'); and then traverse it and assert the fields are what they should be?
(03:17:33 PM) jhodgdon: mpdonadio: that seems like it would be sufficient for the test

jhodgdon’s picture

Issue summary: View changes
Status: Needs review » Needs work

So those are thoughts about what the test would need to do. I'm updating the summary's data modeling piece too.

I also took a look at the interdiff on #97 and it looks good!

+++ b/core/modules/views/views.install
@@ -23,20 +23,17 @@ function views_install() {
+      if (!empty($display['display_options']['row']['type']) && $display['display_options']['row']['type'] === 'fields') {

Just one thing I thought of here: I'm not sure that row type 'fields' is sufficient here. I think maybe some display plugins that use fields (like tables) might have their own row types? Anyway we might need a slightly different test in this line of code to test "Does this display use fields?".

Maybe it should just do isset($display['display_options']['fields'])

and maybe also check that it's an array of non-zero length?

Then I think all we need is a test!

Given the above questions, I am wondering if in the test it might be good to have one display in the test view config that has teasers or something in it (not using fields) just to have a reality check that it will not crash on views that don't use fields?

jhodgdon’s picture

don't need that tag now

The last submitted patch, 97: update_entityviewsdata-2455125-97.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
38.24 KB
20.57 KB

First pass as the update test. Ready for initial review.

I'll fix the formatting in drupal-8.views-entity-views-data-2455125.php when I know I won't have to export/convert that view again.

jhodgdon’s picture

Status: Needs review » Needs work

This looks great! The test seems very thorough to me.

I think it's ready to go except for very minor cosmetic things:

  1. +++ b/core/modules/system/tests/fixtures/update/drupal-8.views-entity-views-data-2455125.php
    @@ -0,0 +1,416 @@
    +// Structure of a custom block with visibility settings.
    

    Fix comment. ;) (it's a view not a custom block)

  2. +++ b/core/modules/system/tests/fixtures/update/drupal-8.views-entity-views-data-2455125.php
    @@ -0,0 +1,416 @@
    \ No newline at end of file
    

    whoops.

  3. And you said you wanted to reformat the exported view... I suppose to use [] array syntax etc.? It didn't look horrible to me.
mpdonadio’s picture

#103-3, the formatting isn't bad (it's from `var_export`, thanks @dkinzer and @neclimdul for the help here), but it doesn't conform to Drupal Coding standards. A few quick search/replaces and autoformatting with PhpStorm should make it quick work. I'll have a fix tonight when I get my "Drupal time" at home.

jhodgdon’s picture

On the other hand, you could make an argument that leaving the var_export() alone could be a good thing for maintainability.

If we eventually decided we needed to add more to this test, and wanted to follow the same procedure you did (make a view, export it with var_export, clean it up), that would be more difficult than if the procedure was (make a view, export it with var_export).

So I would argue that maybe the thing to do is:

a) Attach a text file here with the view config you exported.
b) In the whatever.php file with that export, add a comment saying something like:

----
The following is an export of a View config from comment #106 of https://www.drupal.org/node/2455125 and can be regenerated by ..... [put your instructions here].
----

I mean, it's valid PHP, right? I think we can be a bit relaxed about the coding standards when it comes to something being more maintainable?

mpdonadio’s picture

I'm kinda or wondering why we can't include the YAML as part the text fixture, and parse it to an array with \Drupal\Component\Serialization\Yaml::decocde() directly, and save the conversation to a literal PHP array step?

jhodgdon’s picture

Sounds like an even better idea if you can figure out how to do it!

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
32.11 KB
24.42 KB

#105 taken care of, and added the idea from #106.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Looks great! I think this is ready to go in, and the issue summary is updated too. Let's do it!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 108: update_entityviewsdata-2455125-108.patch, failed testing.

mpdonadio’s picture

Rebase b/c #2351015: URL generation does not bubble cache contexts.

Simple merge in GlossaryTest.php, which now passes locally for me.

mpdonadio’s picture

Status: Needs work » Needs review

Setting the proper status on the right issue would help...

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the reroll.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/views.install
@@ -11,3 +11,90 @@
+    $view->save();

We've been discussing how hook_Update_N and config fit together - see #2538228: Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly. This save should have the $trusted_data = TRUE so schema is not used. Also you need to confirm that all the values you are setting above comply with the schema.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I'll get a new patch posted tonight or tomorrow.

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

RE #114, why is that needed on this patch? The config before and after is both schema-compliant. What we're doing in the update is just changing from using one plugin to a different one -- both plugins existed before and after this patch.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Because using schema during hook_update_N has been deemed problematic - see discussions on #2538228: Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly and #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager). Basically future updates could change the schema resulting in the save breaking.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
31.64 KB

Ah right. I'll add that to the docs:
https://www.drupal.org/node/2535454/revisions/view/8719394/8721764

So here's a quick patch that changes to use save(TRUE).

All I did was edit the patch file and change $view->save() to $view->save(TRUE) in views_update_8001(). I did not make an interdiff file.

mpdonadio’s picture

Is more work needed on this? I read the issues linked in #117 and not seeing schema validation in #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager). When we do

+++ b/core/modules/views/views.install
@@ -11,3 +11,90 @@
+            $date_format = !empty($field['date_format']) ? $field['date_format'] : 'medium';
+            $custom_date_format = !empty($field['custom_date_format']) ? $field['custom_date_format'] : '';
+            $timezone = !empty($field['timezone']) ? $field['timezone'] : '';
+

do we then need to validate these values against what are expected? What happens if we get a bad value? Exception? Abort the update?

/me is confused?

jhodgdon’s picture

I don't think we would need to validate these values. We're taking them from one presumably validated config area and putting them into another?

mpdonadio’s picture

Status: Needs review » Needs work

I want to double check the mappings. I have a suspicion that since we simplified the timeago choices, we need to add some logic to map the text that gets displayed. Will have a new patch or update tomorrow night (too tired now).

mpdonadio’s picture

OK, I think we map everything from the Date field plugin, and the schema types should be good (I did some anti-testing, and it barfs when you try to store the granularity as a string).

Do we need more test coverage? If so, what combos should I add.

mpdonadio’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me. I think we have good test coverage, and now you've ensured the data is saved in the right format, and we are using $config->save(TRUE). So I think this is back to RTBC.

Gábor Hojtsy’s picture

Issue tags: +sprint

Should have been on D8MI sprint for a while :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed cdd12a9 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

Gábor Hojtsy’s picture

Issue tags: -sprint

Superb, thanks!

  • alexpott committed cdd12a9 on 8.0.x
    Issue #2455125 by mpdonadio, rteijeiro, jhodgdon, dawehner: Update...

Status: Fixed » Closed (fixed)

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

mpdonadio’s picture

Assigned: mpdonadio » Unassigned