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
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... |
Comment | File | Size | Author |
---|---|---|---|
#122 | interdiff-118-122.txt | 4.38 KB | mpdonadio |
#122 | update_entityviewsdata-2455125-122.patch | 32.78 KB | mpdonadio |
#118 | update_entityviewsdata-2455125-118.patch | 31.64 KB | jhodgdon |
#111 | update_entityviewsdata-2455125-111.patch | 31.64 KB | mpdonadio |
#108 | interdiff-102-108.txt | 24.42 KB | mpdonadio |
Comments
Comment #1
jhodgdonRemoving one part -- those are just arguments, not field formatters.
Comment #2
jhodgdonAs 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.
Comment #3
xjmComment #4
iMiksuComment #5
iMiksuComment #6
mpdonadioPostponing on #2399211: Support all options from views fields in DateTime formatters. I'll fix this when I am done with that one :)
Comment #7
YesCT CreditAttribution: YesCT commented.
Comment #8
mpdonadioI'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).
Comment #10
dawehnerYeah, we have to switch over in
core/modules/views/src/EntityViewsData.php:311
and then convert all the existing views to point toplugin_id: field
and the required changes of the field options.Comment #11
mpdonadioOK, I'll have a better patch in a day or two.
Comment #12
mpdonadioUnpostponing. They are related, but shouldn't block each other.
Comment #13
mpdonadioOK, here is a better starting point. No worth posting an interdiff. The main change is
in EntityViewsData, and then a lot of
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.
Comment #15
mpdonadioAnd 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.
Comment #16
mpdonadioFirst 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.
Comment #18
mpdonadioOK, let's see if this set of YAML changes goes better.
Comment #20
mpdonadioComment #22
mpdonadioAll 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.
Comment #23
mpdonadioComment #25
mpdonadioFor 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).
Comment #27
mpdonadioOK, field/Date.php is in this patch. I think I have all fails fixed except for
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?
Comment #29
dawehnerI guess we should explain this line?
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.
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 ofcore/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.
Comment #30
mpdonadioMethinks 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.
Comment #31
mpdonadioOK, 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)?
Comment #33
mpdonadioRebased against HEAD. No manual merges. Comments in #31 should still apply.
Comment #35
dawehnerWhat 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?
Comment #36
mpdonadioRebased 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.
Comment #37
alexpottDiscussed 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.
Comment #38
dawehnerMh, 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.
Comment #39
rteijeiro CreditAttribution: rteijeiro commentedRe-rolled and fixed a few nitpicks.
Comment #40
jhodgdonSo... 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?
Comment #41
mpdonadioI 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?
Comment #42
jhodgdonHm. 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.
Comment #43
jhodgdonSimilar discussions about prefix/suffix/format are happening on #2399211: Support all options from views fields in DateTime formatters and #2456521: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known
See #2399211-71: Support all options from views fields in DateTime formatters and #2456521-62: Add DateFormatter::formatDiff() as a non-buggy alternative to DateFormatter::formatInterval() when the start and end of the interval are known
Comment #44
mpdonadioUnassigning myself. My availability the next two or three weeks may be spotty.
Comment #45
mpdonadioOK, 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.
Comment #46
mpdonadioI 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.
Comment #47
mpdonadioSince #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.
Comment #48
mpdonadioUnassigning myself as I have an issue with my MAMP, and someone may be able to work on this at the con.
Comment #49
jhodgdon@mpdonadio: Are you planning/able to get back to this one?
Comment #50
jhodgdonOne 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.
Comment #51
mpdonadioYes; 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.
Comment #52
jhodgdonGreat, thanks!!!
Comment #53
mpdonadioI'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.
Comment #54
mpdonadioPostponing on #2497447: DrupalDateTime::format() and DateTimePlus::format() ignore the timezone setting. (and kinda on #2399211). See also #2497585: Simpletest should set a system timezone in setUp which may affect this.
Comment #55
mpdonadioI 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.
Comment #57
mpdonadioFixing up TimestampFormatterTest was easier than I thought.
Comment #59
mpdonadioI 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.
Comment #60
mpdonadioUnassigning myself. #59 is green, and I think this is ready for review.
Comment #61
dawehnerSo 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
What about using $this->getSetting('timezone') ?: NULL;
Let's use ====
The entire conditions could be one line, and so probably easier to read
Comment #62
mpdonadioComment #63
jhodgdonUm... 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.
Comment #64
jhodgdonI 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.
Comment #65
jhodgdonNo, 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.
Comment #66
mpdonadioSo 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.
Comment #67
jhodgdonWell, 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?
Comment #68
mpdonadioJust 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.
Comment #69
jhodgdonWe can un-postpone this now!!!!!!!!!
Comment #70
mpdonadioYup. I'll have a revised version in a day or two w/ the Timestamp formatters stripped out.
Comment #71
mpdonadioThis 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.
Comment #72
mpdonadioSome new views landed that use the date plugin. Need to do some side testing to find all of them.
Comment #73
mpdonadioI 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?
Comment #74
jhodgdonRight. 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.
Comment #75
jhodgdonBeta 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.
Comment #76
jhodgdonAdding notes about the data model changes.
Comment #77
jhodgdonComment #78
jhodgdonWell, this didn't get in before July 1st, so I guess we need to do something about the update path?
Comment #79
jhodgdonI guess there's a tag.
Comment #80
jhodgdonI talked to Gabor in IRC just now and we think we do not need an upgrade path for this. Hopefully. Explaining in summary.
Comment #81
alexpottBut 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?
Comment #82
mpdonadioOK, 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?
Comment #83
mpdonadioJust fixes the @time => @interval change in a few views. Will think about the rest later...
Comment #84
dawehnerSo this is actually needs work?
Comment #85
mpdonadioYeah, 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.
Comment #86
jhodgdonDo we even know how to do upgrade hooks and tests for this type of change?
Comment #87
mpdonadioIf '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?
Comment #88
dawehnerWe 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.
Comment #89
Gábor Hojtsy@mpdonadio: want to take a stab at this? :)
Comment #90
mpdonadioIn 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.
Comment #91
mpdonadio/grumble
Likely will start this over the weekend...
Comment #92
mpdonadioI think this works, but someone else should manually test it. It's also a little rough. Didn't do the tests yet.
Comment #94
mpdonadioThis 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.
Comment #95
mpdonadioSpent more time with this. I don't really know what / how to test this. Halps!
Comment #96
jhodgdonLooking good! I am not sure about the tests either, but do the issues dawehner referenced above have tests?
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. :)
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.
Comment #97
mpdonadio#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.
Comment #98
jhodgdonmpdonadio 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
Comment #99
jhodgdonSo 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!
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?
Comment #100
jhodgdondon't need that tag now
Comment #102
mpdonadioFirst 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.
Comment #103
jhodgdonThis looks great! The test seems very thorough to me.
I think it's ready to go except for very minor cosmetic things:
Fix comment. ;) (it's a view not a custom block)
whoops.
Comment #104
mpdonadio#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.
Comment #105
jhodgdonOn 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?
Comment #106
mpdonadioI'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?
Comment #107
jhodgdonSounds like an even better idea if you can figure out how to do it!
Comment #108
mpdonadio#105 taken care of, and added the idea from #106.
Comment #109
jhodgdonLooks great! I think this is ready to go in, and the issue summary is updated too. Let's do it!
Comment #111
mpdonadioRebase b/c #2351015: URL generation does not bubble cache contexts.
Simple merge in GlossaryTest.php, which now passes locally for me.
Comment #112
mpdonadioSetting the proper status on the right issue would help...
Comment #113
Gábor HojtsyThanks for the reroll.
Comment #114
alexpottWe'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.
Comment #115
mpdonadioI'll get a new patch posted tonight or tomorrow.
Comment #116
jhodgdonRE #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.
Comment #117
alexpottBecause 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.
Comment #118
jhodgdonAh 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.
Comment #119
mpdonadioIs 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
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?
Comment #120
jhodgdonI 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?
Comment #121
mpdonadioI 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).
Comment #122
mpdonadioOK, 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.
Comment #123
mpdonadioComment #124
jhodgdonThis 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.
Comment #125
Gábor HojtsyShould have been on D8MI sprint for a while :)
Comment #126
alexpottCommitted cdd12a9 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #127
Gábor HojtsySuperb, thanks!
Comment #130
mpdonadio