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

  1. Update the code for flattening values
  2. Ensure no markup is in place
  3. Decide what to do about multi-value fields
  4. Compare export results to avoid breaking uses of current exports
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

scott_euser created an issue. See original summary.

scott_euser’s picture

My 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:

Compare export results to avoid breaking uses of current exports

mbovan’s picture

I'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 and datetime 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.

Status: Needs review » Needs work

The last submitted patch, 3: consider_changing_the-2853756-3.patch, failed testing.

mbovan’s picture

Correct patch directory.

Berdir’s picture

+++ b/modules/contrib/contact_storage_export/src/ContactStorageExportService.php
@@ -58,171 +72,157 @@ class ContactStorageExportService implements ContainerInjectionInterface {
+        default:
+          // Display a field item value using default field formatter.
+          $renderable_array = $item->view();
+          $values[] = $this->renderAsString($renderable_array);
+      }

wondering 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()

mbovan’s picture

Created 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.

Status: Needs review » Needs work

The last submitted patch, 7: consider_changing_the-2853756-7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
scott_euser’s picture

This 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.

The last submitted patch, 7: consider_changing_the-2853756-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: consider_changing_the-2853756-9.patch, failed testing.

Berdir’s picture

Re 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?

scott_euser’s picture

Thanks 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.

scott_euser’s picture

Status: Needs work » Needs review

Switching to needs review to make sure the test still passes.

  • scott_euser committed 449393e on 8.x-1.x authored by mbovan
    Issue #2853756 by mbovan, Berdir: Consider changing the way values are...
scott_euser’s picture

Status: Needs review » Fixed
scott_euser’s picture

Issue tags: +DrupalCampLDN

Status: Fixed » Closed (fixed)

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