[PoC] Value formatters #649

phansys · 2023-10-23T14:35:19Z

Subject

Introduce value formatters.

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).
Update the documentation;
Complete the PR's description;
Add more formatters (like SerializableFormatter, TranslatableFormatter, etc);
Add an upgrade note;
Update the tests;
Fix the findings detected in the CI pipeline;
~~Determine how to cover the recursion required by the IterableFormatter formatter;~~

core23 · 2023-10-23T15:08:32Z

src/Formatter/BoolFormatter.php

+
+    public function __construct(
+        private string $trueLabel = self::LABEL_TRUE,
+        private string $falseLabel = self::LABEL_FALSE


Can you make this translatable?

Yes, but I think this should be responsibility of something like a TranslationFormatter. WDYT?

core23 · 2023-10-23T15:09:54Z

src/Formatter/IterableFormatter.php

+                $value = iterator_to_array($value);
+            }
+
+            $data[$key] = '['.implode(', ', array_map([$this, 'formatFromIterable'], $value)).']';


I know we have this for quite a while, but do we need the brackets?

I think the brackets are used here to represent an array structure instead of a colloquial list.

phansys

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

Thank you in advance.

VincentLanglet · 2023-12-11T20:38:42Z

@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 · 2023-12-11T21:39:51Z

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 · 2023-12-12T08:31:59Z

src/Source/DoctrineORMQuerySourceIterator.php

        private int $batchSize = 100,
-        bool $useBackedEnumValue = true,
+        ?bool $useBackedEnumValue = null,
+        bool $disableSourceFormatters = true


Why is it false by default in the abstract and true by default here ?

My bad, thanks (I guess it was preventing the new deprecations).
I'll be updating this soon.

VincentLanglet · 2023-12-12T08:34:45Z

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

phansys · 2023-12-12T17:18:16Z

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 · 2023-12-13T14:57:38Z

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:

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;
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.
Improve the integration from SonataAdmin, in order to have an "export" API similar to the others ("list", "show", etc).

VincentLanglet · 2023-12-13T17:28:58Z

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:

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 · 2023-12-13T19:45:39Z

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 · 2024-04-12T15:42:57Z

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

phansys added in progress feature labels Oct 23, 2023

core23 reviewed Oct 23, 2023

View reviewed changes

phansys force-pushed the value_formatters branch 21 times, most recently from 6decc95 to 896d4f6 Compare December 11, 2023 19:32

phansys requested review from core23 and a team December 11, 2023 19:34

phansys commented Dec 11, 2023

View reviewed changes

phansys force-pushed the value_formatters branch from 896d4f6 to 87eae9f Compare December 11, 2023 20:04

phansys force-pushed the value_formatters branch 4 times, most recently from 4b54296 to 4f1deba Compare December 12, 2023 01:45

VincentLanglet reviewed Dec 12, 2023

View reviewed changes

phansys force-pushed the value_formatters branch from 4f1deba to 6d39c06 Compare December 12, 2023 17:13

phansys force-pushed the value_formatters branch from 6d39c06 to 6664124 Compare December 13, 2023 02:18

Introduce value formatters

c83714c

phansys force-pushed the value_formatters branch from 6664124 to c83714c Compare December 13, 2023 02:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PoC] Value formatters #649

[PoC] Value formatters #649

phansys commented Oct 23, 2023 •

edited

core23 Oct 23, 2023

phansys Oct 23, 2023

core23 Oct 23, 2023

phansys Oct 23, 2023

phansys left a comment

VincentLanglet commented Dec 11, 2023

phansys commented Dec 11, 2023

VincentLanglet Dec 12, 2023

phansys Dec 12, 2023

VincentLanglet commented Dec 12, 2023

phansys commented Dec 12, 2023

phansys commented Dec 13, 2023

VincentLanglet commented Dec 13, 2023

phansys commented Dec 13, 2023

Hanmac commented Apr 12, 2024 •

edited

[PoC] Value formatters #649

Are you sure you want to change the base?

[PoC] Value formatters #649

Conversation

phansys commented Oct 23, 2023 • edited

Subject

Changelog

To do

core23 Oct 23, 2023

Choose a reason for hiding this comment

phansys Oct 23, 2023

Choose a reason for hiding this comment

core23 Oct 23, 2023

Choose a reason for hiding this comment

phansys Oct 23, 2023

Choose a reason for hiding this comment

phansys left a comment

Choose a reason for hiding this comment

VincentLanglet commented Dec 11, 2023

phansys commented Dec 11, 2023

VincentLanglet Dec 12, 2023

Choose a reason for hiding this comment

phansys Dec 12, 2023

Choose a reason for hiding this comment

VincentLanglet commented Dec 12, 2023

phansys commented Dec 12, 2023

phansys commented Dec 13, 2023

VincentLanglet commented Dec 13, 2023

phansys commented Dec 13, 2023

Hanmac commented Apr 12, 2024 • edited

phansys commented Oct 23, 2023 •

edited

Hanmac commented Apr 12, 2024 •

edited