sonata-project / exporter

Lightweight Exporter library
https://docs.sonata-project.org/projects/exporter
MIT License
439 stars 88 forks source link

[PoC] Value formatters #649

Open phansys opened 1 year ago

phansys commented 1 year ago

Subject

Introduce value formatters.

See https://github.com/sonata-project/exporter/issues/293#issuecomment-1774071438.

I am targeting this branch, because these changes should respect BC.

Closes #293.

Changelog

### Added
- `Sonata\Exporter\Formatter\BoolFormatter`, `Sonata\Exporter\Formatter\DateIntervalFormatter`, `Sonata\Exporter\Formatter\DateTimeFormatter`, `Sonata\Exporter\Formatter\EnumFormatter`, `Sonata\Exporter\Formatter\IterableFormatter`, `Sonata\Exporter\Formatter\StringableFormatter` and
  `Sonata\Exporter\Formatter\SymfonyTranslationFormatter`
  classes to be used within implementations of `Sonata\Exporter\Formatter\Writer\FormatAwareInterface`
- `sonata_exporter.writers.{writer}.formatters` configuration in order to determine which formatters will be used by each writer.
  By default, "bool", "dateinterval", "datetime", "enum", "iterable" and "stringable" formatters are configured.
  If "symfony/translations-contracts" is installed, "symfony_translator" formatter is also enabled.

### Deprecated
- `Sonata\Exporter\Writer\FormattedBoolWriter`, use `Sonata\Exporter\Formatter\BoolFormatter` instead.
- Arguments `dateTimeFormat` and `useBackedEnumValue` in `Sonata\Exporter\Source\AbstractPropertySourceIterator::__construct()` and
  their children classes. To disable the source formatting you MUST pass `true` in argument `disableSourceFormatters` and use
  `Sonata\Exporter\Formatter\Writer\FormatAwareInterface::addFormatter()` in your writers instead.

To do

VincentLanglet commented 11 months ago

@sonata-project/contributors, I'd like to know your opinion about this approach before spending more time.

Thank you in advance.

I like the idea. But I'm not sure about the

protected function format(array $data): array
    {
        foreach ($this->formatters as $formatter) {
            $data = $formatter->format($data);
        }

        return $data;
    }

part.

If we have string formatter, we will have a risk to have some conflict with previous formatter (which already format some non-string data like boolean or date, to string).

Shouldn't we have something like

foreach ($this->formatters as $formatter) {
     if ($formatter->support($data);
            return $formatter->format($data);
     }
}
phansys commented 11 months ago

Thanks for the quick reply.

If we have string formatter, we will have a risk to have some conflict with previous formatter (which already format some non-string data like boolean or date, to string).

I have the same concern, the intention of this item in the To Do list was to cover that:

Evaluate if the concept of priority is required (by instance, to translate a value after a previous formatting).

About the supports() method, I'm currently implementing these checks in an implicit way. See this example. BTW, please be aware that the $data variable in $formatter->support($data) is an array representing a row and not a single item.

VincentLanglet commented 11 months ago

About the supports() method, I'm currently implementing these checks in an implicit way. See this example. BTW, please be aware that the $data variable in $formatter->support($data) is an array representing a row and not a single item.

Ok, so I thought about something like

foreach ($data as $key => $value) {
     foreach ($this->formatters as $formatter) {
         if ($formatter->support($key, $value);
            $data[$key] = $formatter->format($key, $value);
            continue 2;
         }
    }
}

This way a value is formatted only once.

And we could provide a ChainFormatter if someone want to apply multiple formatter on a specific value. (Like Boolean + Translator). This way, If I export

I'll get "trans(true)" for the boolean (If I used a ChainFormatter) without impacting the "true" string.

Not sure if I'm clear @phansys

phansys commented 11 months ago

Not sure if I'm clear @phansys

I think I get your point, thank you. I'll be pushing a new commit when I have something about this approach.

phansys commented 11 months ago

For the records, I'm dumping here some thoughts I currently have about this feature, but I'd like to debate later if this PR is merged:

  1. Possibility to have mappings in the sources, allowing to determine what "columns" in the exported values must be processed by each formatter. This way, we will save iterations on values that are not covered by a formatter;
  2. Support for templates (like Twig), in order to let the user to have more control on specific cases. This should bring a DX similar than the one we have for the admin mappers in SonataAdmin.
  3. Improve the integration from SonataAdmin, in order to have an "export" API similar to the others ("list", "show", etc).
VincentLanglet commented 11 months ago

For the records, I'm dumping here some thoughts I currently have about this feature, but I'd like to debate later if this PR is merged:

  1. Possibility to have mappings in the sources, allowing to determine what "columns" in the exported values must be processed by each formatter. This way, we will save iterations on values that are not covered by a formatter;

That's why I wrote the code

if ($formatter->support($key, $value);
            $data[$key] = $formatter->format($key, $value);
            continue 2;
         }

By passing the key to suport/format method, it allows to have different behavior based on the column.

phansys commented 11 months ago

By passing the key to suport/format method, it allows to have different behavior based on the column.

Yes, but in this case $key is the column name. We still need a mapping to provide the type for each column name. In that case, if you suggest adding something like that in this PR, I guess we should also update the sources in order to return the mapping.

IMO, building the mapping will not be so easy. In case of objects (like the ones returned by AbstractPropertySourceIterator) we could use the reflection API to guess the types, but in case of sources returning arrays it may be harder.

Hanmac commented 7 months ago

@phansys first i wanted to make an extra Issue about this, but then i thought i might comment it there too:

when doing the DatetimeFormater, could you maybe add a Timezone change too?

Like for example the Data is stored in UTC Time, but the Grid uses Sonata\IntlBundle to show it in User Time. But right now, the Exporter doesn't respect that.

could there be a Hook where Sonata\IntlBundle\Timezone\TimezoneDetectorInterface can be connected to change the Timezone for the Export output?

Or would it be easier to just overwrite sonata.exporter.formatter.datetime in user code?

Edit: OR also change the Datetime Format depending on the User Locale?

github-actions[bot] commented 1 month ago

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Hanmac commented 1 month ago

A different idea I had right now is to feed the export data into Symfony Serializer

Then we could use all the Normalizer Features we want

But I need to brainstorm about that idea


Hm for my Serializer idea, the getDataSourceIterator sadly already has translated keys, and AbstractPropertySourceIterator already has values like DateTime turned into Strings I would have liked, if they had been translated later, after my Serializer would have worked with their keys