NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

Lctrs wants to merge8 commits intosymfony:3.4fromLctrs:feature/dateinterval-normalizer

Closed

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

Lctrs wants to merge8 commits intosymfony:3.4fromLctrs:feature/dateinterval-normalizer

Conversation

Copy link

Contributor

Lctrs commentedAug 1, 2017•
edited
Loading

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.

carsonbot added Status: Needs Review Serializer FrameworkBundle Feature labels

Aug 1, 2017

ogizanagi reviewed

Aug 2, 2017

View reviewed changes

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

		{
		if (class_exists('Symfony\Component\Serializer\Normalizer\DataUriNormalizer')) {
		// Runafter serializer.normalizer.object
		// Runbefore serializer.normalizer.object

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

LctrsAug 2, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

Done in#23765.

ogizanagi added this to the3.4 milestone

Aug 2, 2017

ogizanagi approved these changes

Aug 8, 2017

View reviewed changes

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

		}

		if (!$this->isISO8601($data)) {
		thrownewUnexpectedValueException('Non ISO 8601 interval strings are not supported yet.');

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Well, as I said, most code have been adapted from the DateInterval form type, and especially from the classDateIntervalToStringTransformer 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.

Copy link

Member

chalasrAug 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

LctrsAug 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree.

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

Lctrs mentioned this pull request

Aug 9, 2017

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

Closed

Lctrs commented

Aug 13, 2017

View reviewed changes

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

Copy link

ContributorAuthor

LctrsAug 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

ogizanagiAug 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Looks good to me :)

Copy link

ContributorAuthor

Lctrs commentedAug 13, 2017

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

Copy link

ContributorAuthor

Lctrs commentedAug 18, 2017

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

ogizanagi reviewed

Aug 18, 2017

View reviewed changes

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

		publicfunctiondenormalize($data,$class,$format =null,array$context =array())
		{
		if (!is_string($data)) {
		thrownewInvalidArgumentException('Data expected to be a string,'.gettype($data).' given.');

Copy link

Contributor

ogizanagiAug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You should usesprintf for exception messages.

ogizanagi requested a review fromdunglas

August 18, 2017 06:10

Copy link

Contributor

ogizanagi commentedAug 31, 2017

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

Copy link

ContributorAuthor

Lctrs commentedAug 31, 2017

@ogizanagi Done.

Jérôme Parmentierand others added7 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

Copy link

ContributorAuthor

Lctrs commentedSep 14, 2017

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

ogizanagi reviewed

Sep 14, 2017

View reviewed changes

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

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

Copy link

Contributor

ogizanagiSep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

		if ('workflow' ===$type) {
		$transitions[] =newDefinition(Workflow\Transition::class,array($transition['name'],$transition['from'],$transition['to']));
		}elseif ($type ==='state_machine') {
		}elseif ('state_machine' ===$type) {

Copy link

Contributor

ogizanagiSep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Same

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

		if (ChainAdapter::class ===$parentDefinition->getClass()) {
		break;
		}
		// no break

Copy link

Contributor

ogizanagiSep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Same

Copy link

Contributor

ogizanagi commentedSep 14, 2017•
edited
Loading

@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 :)

Partially revert previous commit

db7e9cc

Copy link

Contributor

ogizanagi commentedSep 15, 2017

Thank you@Lctrs.

ogizanagi added a commit that referenced this pull request

Sep 15, 2017

feature#23747[Serializer][FrameworkBundle] Add a DateInterval norma…

c7e84cc

…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#8267Could 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

ogizanagi closed this

Sep 15, 2017

Lctrs deleted the feature/dateinterval-normalizer branch

September 15, 2017 16:11

xabbuh added a commit to symfony/symfony-docs that referenced this pull request

Sep 22, 2017

minor#8267[Serializer] Document the new DateIntervalNormalizer norm…

4f94c5e

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

This was referencedOct 18, 2017

Release v3.4.0-BETA1#24609

Merged

Release v4.0.0-BETA1#24612

Merged

Labels

Feature FrameworkBundle Serializer Status: Needs Review

4 participants

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

Lctrs commentedAug 1, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LctrsAug 2, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lctrs commentedAug 13, 2017

Uh oh!

Lctrs commentedAug 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi commentedAug 31, 2017

Uh oh!

Lctrs commentedAug 31, 2017

Uh oh!

Lctrs commentedSep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagi commentedSep 14, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ogizanagi commentedSep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Lctrs commentedAug 1, 2017•
edited
Loading

LctrsAug 2, 2017•
edited
Loading

ogizanagi commentedSep 14, 2017•
edited
Loading