exporter icon indicating copy to clipboard operation
exporter copied to clipboard

[PoC] Value formatters

Open phansys opened this issue 2 years ago • 11 comments

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

  • [ ] Validate the proposal;
  • [ ] Evaluate if the concept of priority is required (by instance, to translate a value after a previous formatting).
  • [x] Update the documentation;
  • [x] Complete the PR's description;
  • [x] Add more formatters (like SerializableFormatter, TranslatableFormatter, etc);
  • [x] Add an upgrade note;
  • [x] Update the tests;
  • [x] Fix the findings detected in the CI pipeline;
  • [x] ~~Determine how to cover the recursion required by the IterableFormatter formatter;~~

phansys avatar Oct 23 '23 14:10 phansys

@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);
     }
}

VincentLanglet avatar Dec 11 '23 20:12 VincentLanglet

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.

phansys avatar Dec 11 '23 21:12 phansys

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

  • A boolean TRUE
  • A string 'true'

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

VincentLanglet avatar Dec 12 '23 08:12 VincentLanglet

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 avatar Dec 12 '23 17:12 phansys

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

phansys avatar Dec 13 '23 14:12 phansys

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.

VincentLanglet avatar Dec 13 '23 17:12 VincentLanglet

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.

phansys avatar Dec 13 '23 19:12 phansys

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

Hanmac avatar Apr 12 '24 15:04 Hanmac