Problem/Motivation
As raised by Berdir in 2853480 in regards to flattening values:
Note that we took a bit a different approach for this e.g. in monitoring, see \Drupal\monitoring\Plugin\monitoring\SensorPlugin\ContentEntityAggregatorSensorPlugin::verboseResultUnaggregated()
Basically, we use the default formatter for a field type and if not given, we fall back to the main property.
Using the formatter works much better for fields like dates, lists, references, where you don't want the raw value/target_id/... but something formatted, e.g. the date in the right timezone and properly formatted, or the label of the referenced entity.
Obviously we'd have to remove the markup again and probably figure out what to do about multi-value fields.
Proposed resolution
Use the approach in the monitoring project to flatten the data as shown here http://cgit.drupalcode.org/monitoring/tree/src/Plugin/monitoring/SensorP...
Remaining tasks
- Update the code for flattening values
- Ensure no markup is in place
- Decide what to do about multi-value fields
- Compare export results to avoid breaking uses of current exports
Comment | File | Size | Author |
---|---|---|---|
#10 | consider_changing_the-2853756-9.patch | 21.29 KB | scott_euser |
#7 | consider_changing_the-2853756-7-interdiff-2853480.13.txt | 21.29 KB | mbovan |
#7 | consider_changing_the-2853756-7-interdiff-5.txt | 6.04 KB | mbovan |
#7 | consider_changing_the-2853756-7.patch | 23.11 KB | mbovan |
#5 | consider_changing_the-2853756-3-interdiff.txt | 20.3 KB | mbovan |
Comments
Comment #2
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedMy initial thoughts are that I need to put tests in place first covering a range of field types and data in those field types before attempting the change because of:
Comment #3
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI'm uploading a patch that leverages the idea @Berdir pointed-out.
It works quite nice IMHO... However, some fields needed special handling, so I implemented it for
entity_reference
,link
anddatetime
field types for now.The patch was based on top of #2853480: Expose export helpers through a service (#11).
The interdiff above represents the actual changes between this issue and #11.
Comment #5
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedCorrect patch directory.
Comment #6
Berdirwondering if we want to do something like token.module. Support a contact_storage_export view mode that you can enable and configure per contact form and fall back to the default settings. Also, don't we get labels if we do it like this, or are you working around that by rendering each item separately?
See field_tokens()
Comment #7
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedCreated followup for #6: #2856956: Improve export configurability with a custom view mode
In #5, I removed
date_format
setting and used default field formatter setting instead.Since this setting is available on the Export Form, I reverted the changes and made all date-like (datetime, created, timestamp, daterange) fields consider
date_format
.Comment #9
BerdirComment #10
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedThis makes sense to me and produces roughly the same output, but improved in some cases (eg, boolean's output 'Yes' instead of '1'). It causes a bit of a backwards compatibility issue that I need to think about. Perhaps making a 8.x-2.x-alpha/dev branch and an explanation that it is compatible but the output is different (better). Would appreciate if you guys have any advice on that.
Re-uploading the patch after commit on dev branch for 2853480.
Comment #13
BerdirRe 8.x-2.x. that's up to you, we'd be OK with that.
It is maintenance overhead, especially if you plan to maintain both versions (for how long?) Or would you just use the major version switch to clarify to users that there are changes and stop supporting 1.x soon-ish?
Comment #14
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedThanks for the advice. To avoid the maintenance I think I will leave it on the same branch. Anyone extending the module itself won't have an issue, only someone who is download the CSV and importing it into another external system might notice a slight change (improvement) in the exported information.
Comment #15
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedSwitching to needs review to make sure the test still passes.
Comment #17
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commentedComment #18
scott_euser CreditAttribution: scott_euser as a volunteer and at Fat Beehive commented