[Serializer][FrameworkBundle] Add a DateInterval normalizer #23747

Lctrs · 2017-08-01T15:37:33Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	symfony/symfony-docs#8267

Could be useful for API needing to submit a duration.

Most code have been adapted from @MisatoTremor's DateInterval form type. Credits to him.

ogizanagi · 2017-08-02T14:12:54Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -1336,12 +1337,19 @@ private function registerSecurityCsrfConfiguration(array $config, ContainerBuild
    private function registerSerializerConfiguration(array $config, ContainerBuilder $container, XmlFileLoader $loader)
    {
        if (class_exists('Symfony\Component\Serializer\Normalizer\DataUriNormalizer')) {
-            // Run after serializer.normalizer.object
+            // Run before serializer.normalizer.object


Could you submit another PR to fix this in 3.3 instead?

👍

Done in #23765.

ogizanagi · 2017-08-08T22:04:58Z

src/Symfony/Component/Serializer/Normalizer/DateIntervalNormalizer.php

+        }
+
+        if (!$this->isISO8601($data)) {
+            throw new UnexpectedValueException('Non ISO 8601 interval strings are not supported yet.');


Why "yet"? Do you plan to add support for this and how?
On the contrary, I'd change the message for "Expected a valid ISO 8601 interval string." and remove related tests/dataprovider.

Well, as I said, most code have been adapted from the DateInterval form type, and especially from the class DateIntervalToStringTransformer which have the same exception message.

I thought that if non ISO 8601 interval string support was added to this class, maybe we could port it to the DateIntervalNormalizer. But I'm not sure it's worth it or a valid use case for the Serializer part.

Agree with @ogizanagi, let's not promise something that we're not sure to want.

I agree.

I'll try to make the requested changes before I go on holidays.

Lctrs · 2017-08-13T22:57:27Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+            // Run before serializer.normalizer.object
+            $definition = $container->register('serializer.normalizer.dateinterval', DateIntervalNormalizer::class);
+            $definition->setPublic(false);
+            $definition->addTag('serializer.normalizer', array('priority' => -915));


@ogizanagi are you okay with this priority ? Or should I change it to -930 to be more consistent with other normalizers ?

Looks good to me :)

Lctrs · 2017-08-13T23:00:19Z

PR updated to fix comment made by @ogizanagi and @chalasr.

Lctrs · 2017-08-18T02:08:44Z

Unless there are more comments that need to be addressed, it should be ready to merge then :)

ogizanagi · 2017-08-18T06:07:40Z

src/Symfony/Component/Serializer/Normalizer/DateIntervalNormalizer.php

+    public function denormalize($data, $class, $format = null, array $context = array())
+    {
+        if (!is_string($data)) {
+            throw new InvalidArgumentException('Data expected to be a string, '.gettype($data).' given.');


You should use sprintf for exception messages.

ogizanagi · 2017-08-31T11:08:02Z

Could you add an entry in component's CHANGELOG.md file?

Lctrs · 2017-08-31T11:44:11Z

@ogizanagi Done.

Lctrs · 2017-09-14T15:49:02Z

@ogizanagi Is there anything else that prevent it from being merged ?

ogizanagi · 2017-09-14T16:17:06Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -631,9 +632,9 @@ private function registerWorkflowConfiguration(array $config, ContainerBuilder $

            $transitions = array();
            foreach ($workflow['transitions'] as $transition) {
-                if ($type === 'workflow') {
+                if ('workflow' === $type) {


Must be reverted. This change does not belong to this PR.

ogizanagi · 2017-09-14T16:17:15Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

                    $transitions[] = new Definition(Workflow\Transition::class, array($transition['name'], $transition['from'], $transition['to']));
-                } elseif ($type === 'state_machine') {
+                } elseif ('state_machine' === $type) {


ogizanagi · 2017-09-14T16:17:30Z

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.php

@@ -1088,6 +1104,7 @@ private function assertCachePoolServiceDefinitionIsCreated(ContainerBuilder $con
                if (ChainAdapter::class === $parentDefinition->getClass()) {
                    break;
                }
+                // no break


ogizanagi · 2017-09-14T16:19:15Z

@Lctrs : Nothing to me (appart from the comments I've just added, since the PR was tweaked since my last review). We just miss another approval (not mandatory here, but always good to take).

ping @symfony/deciders , @dunglas :)

ogizanagi · 2017-09-15T16:09:00Z

Thank you @Lctrs.

@MisatoTremor

…lizer (Lctrs) This PR was squashed before being merged into the 3.4 branch (closes #23747). Discussion ---------- [Serializer][FrameworkBundle] Add a DateInterval normalizer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | symfony/symfony-docs#8267 Could be useful for API needing to submit a duration. Most code have been adapted from @MisatoTremor's DateInterval form type. Credits to him. Commits ------- 6185cb1 [Serializer][FrameworkBundle] Add a DateInterval normalizer

…alizer (Lctrs) This PR was squashed before being merged into the 3.4 branch (closes #8267). Discussion ---------- [Serializer] Document the new DateIntervalNormalizer normalizer Waiting for symfony/symfony#23747 to be merged. Commits ------- 9784714 [Serializer] Document the new DateIntervalNormalizer normalizer

carsonbot added Status: Needs Review Serializer FrameworkBundle Feature labels Aug 1, 2017

ogizanagi reviewed Aug 2, 2017

View reviewed changes

ogizanagi added this to the 3.4 milestone Aug 2, 2017

ogizanagi approved these changes Aug 8, 2017

View reviewed changes

Lctrs mentioned this pull request Aug 9, 2017

[Serializer] Document the new DateIntervalNormalizer normalizer symfony/symfony-docs#8267

Closed

Lctrs commented Aug 13, 2017

View reviewed changes

ogizanagi reviewed Aug 18, 2017

View reviewed changes

ogizanagi requested a review from dunglas August 18, 2017 06:10

Jérôme Parmentier and others added 7 commits September 14, 2017 16:43

Add a DateInterval normalizer

b28839b

Fix wrong format in tests

e86f5a1

Fix wrong comments

643ea73

Update non ISO 8601 interval string exception message + tests

71e8fa7

Switch to sprintf()

2d23e5c

Update CHANGELOG.md

ec7f37f

CS

36acaf0

ogizanagi reviewed Sep 14, 2017

View reviewed changes

Partially revert previous commit

db7e9cc

ogizanagi closed this Sep 15, 2017

Lctrs deleted the feature/dateinterval-normalizer branch September 15, 2017 16:11

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Serializer][FrameworkBundle] Add a DateInterval normalizer #23747

[Serializer][FrameworkBundle] Add a DateInterval normalizer #23747

Lctrs commented Aug 1, 2017 •

edited

Loading

ogizanagi Aug 2, 2017

Lctrs Aug 2, 2017 •

edited

Loading

ogizanagi Aug 8, 2017

Lctrs Aug 9, 2017

chalasr Aug 12, 2017

Lctrs Aug 12, 2017

Lctrs Aug 13, 2017

ogizanagi Aug 14, 2017

Lctrs commented Aug 13, 2017

Lctrs commented Aug 18, 2017

ogizanagi Aug 18, 2017

ogizanagi commented Aug 31, 2017

Lctrs commented Aug 31, 2017

Lctrs commented Sep 14, 2017

ogizanagi Sep 14, 2017

ogizanagi Sep 14, 2017

ogizanagi Sep 14, 2017

ogizanagi commented Sep 14, 2017 •

edited

Loading

ogizanagi commented Sep 15, 2017

[Serializer][FrameworkBundle] Add a DateInterval normalizer #23747

[Serializer][FrameworkBundle] Add a DateInterval normalizer #23747

Conversation

Lctrs commented Aug 1, 2017 • edited Loading

Choose a reason for hiding this comment

Lctrs Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lctrs commented Aug 13, 2017

Lctrs commented Aug 18, 2017

Choose a reason for hiding this comment

ogizanagi commented Aug 31, 2017

Lctrs commented Aug 31, 2017

Lctrs commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Sep 14, 2017 • edited Loading

ogizanagi commented Sep 15, 2017

Lctrs commented Aug 1, 2017 •

edited

Loading

Lctrs Aug 2, 2017 •

edited

Loading

ogizanagi commented Sep 14, 2017 •

edited

Loading