As per #22444 the current behavior is the intended one, isnt it? cc @ogizanagi

It's a BC break IMHO.

@ro0NL the current behaviour only normalizes input if it's already normalized (everything in the same time zone or everything without a time zone). Having any sort of variance means your data will not be normalized in a way you'd expect, which is not something a normalizer should do by default.

PHP itself has the same problem which is the root cause of this issue. See Twitter thread.

So, technically yes, this changes the current behaviour, but the current behaviour is plainly buggy for the outlined reasons. If you explicitly specify a timezone, you should be able to rely on the output being in that timezone.

this is mostly a matter of perspective isnt it, when denormalizing preserving the timezone as-given might be considered expected as well ...

i agree some inconsistency comes with normalize(), where the timezone is always applied; as we don't know the user set it / or has a default timezone.

Would a second context option be the better approach? E.g. preserve_denormalized_timezone = true

@ro0NL doesn't seem like a matter of perspective to me.

If you wish to preserve the timezone given in the input, you can omit specifying a timezone to the normalizer:

1. if a timezone is given, it's preserved
2. if no timezone is given, it will default to PHP's current default timezone

If you, on the other hand, wish to normalize the timezone (with the tool called DatetimeNormalizer, mind you), you currently have no recourse except writing your own normalizer.

i figure it's the most expected to cover timezone normalization consistently in both directions, esp. if one provides a timezone explicitly. I agree it should be omitted otherwise.

Otherwise this option suffers poor naming (opposed to default_timezone).

Should we migrate users from the current behavior to the new one using some flag? while deprecating not setting it to true.

I was leaning towards the view @ro0NL that some type of flag be employed to capture BC. Remember that $defaultContext was only added to the DateTimeNormaliser in version 4.2.0, and as it has the ability to set the default_timezone, previous version of Symfony/Serializer are not impacted by this change.

What should such a flag be called?

• FORCE_TIMEZONE,
• FORCE_DEFAULT_TIMEZONE,
• FORCE_CONTEXT_TIMEZONE,
• other choices.

I have added a flag and tested at my end. I have use a deprecation message of:

'The denormalizer function can produce inconsistent timezone results with this flag set to false. Use a true flag with default_timezone === NULL for default PHP behaviour.'

Will commit when name and message consensus is reached.

Just wondering @ro0NL why we need to deprecate the old way, when we can offer both ways and document appropriately. Would this meet requirements of issue #31232 @dkarlovi

@crayner i'm kinda convinced by #31256 (comment), we should avoid the variance between normalize() and denormalize()

• you pass the timezone, both directions normalize the timezone
• you omit the timezone, both directions dont normalize the timezone

that means when you omit it, you should set the DateTime timezone yourself and it's preserved in both directions (depending on the datetime format in this case)

@crayner we need to deprecate the current behaviour (passing a timezone, but (by relying on the current buggy behaviour) expecting it to not be used. That means you deprecate that usecase, and in this case you do so by having the developer acknowledge they understand the new behaviour and opt-in to it.

So,the new behaviour should be:

1. no timezone passed, keeps old behaviour
2. timezone passed, FORCE_CONTEXT_TIMEZONE not set means the developer has not yet opt-in, throw a deprecation, keep old behaviour
3. timezone passed, FORCE_CONTEXT_TIMEZONE set means the developer had the chance to review the change and is opting-in, use new behaviour

Symfony 5 should use the new behaviour by default without setting FORCE_CONTEXT_TIMEZONE, the flag itself can be deprecated. So I definitely don't see this as a feature, it's a bugfix and the flag is just to allow for the transition phase.

I confirm this was the expected behavior in #22444.
So it doesn't qualify as a bug (and if it was, we won't need an upgrade path).

The use-case was indeed to only fulfill the timezone info when none is provided in the input and preserve the ones explicitly provided.
It was reflecting PHP behaviors:

>>> new \DateTime('2016-01-27T16:39:26+01:00', new \DateTimezone('UTC'));
# => DateTime @1453909166 {#2313
#      date: 2016-01-27 16:39:26.0 +01:00,
#    }

It ignores the second argument. Which surely has value, but perhaps a poor choice / suffers from a naming issue.

So #31256 (comment):

If you wish to preserve the timezone given in the input, you can omit specifying a timezone to the normalizer:

1. if a timezone is given, it's preserved
2. if no timezone is given, it will default to PHP's current default timezone

is true, but the feature allows to explicitly choose the default timezone to use depending on the context while keeping provided timezone info from the input, if any.
Hence, DEFAULT_TIMEZONE would probably have been a better naming, but only for denormalization, not normalization where "default" makes no sense (as the timezone info is already embedded in the \DateTime object).

#31256 (comment):

So,the new behaviour should be:

• no timezone passed, keeps old behaviour
• timezone passed, FORCE_CONTEXT_TIMEZONE not set means the developer has not yet opt-in, throw a deprecation, keep old behaviour
• timezone passed, FORCE_CONTEXT_TIMEZONE set means the developer had the chance to review the change and is opting-in, use new behaviour
  Symfony 5 should use the new behaviour by default without setting FORCE_CONTEXT_TIMEZONE, the flag itself can be deprecated

Then it means the "keeping provided timezone from input" use-case is dropped in 5.0, as well as the FORCE_CONTEXT_TIMEZONE flag becoming no-op and having to be deprecated and dropped in 6.0.

Instead, we introduce a [KEEP/PRESERVE]_INPUT_TIMEZONE flag, set to true by default, which would only have effect on denormalization:

1. In 4.4: A deprecation will be thrown if this flag isn't set by the user in the context or default context, asking the user to acknowledge by using PRESERVE_INPUT_TIMEZONE => true and notifying the default value will be false in 5.0.
   One can opt-out and get the behavior you expect by using PRESERVE_INPUT_TIMEZONE => false
2. In 5.0, just change default value to false and remove the deprecation.

This will keep both use-cases, and no flag to remove later.

@ogizanagi

the feature allows to explicitly choose the default timezone to use depending on the context while keeping provided timezone info from the input, if any.

My reasoning for this being a bug was this can be achieved without any context flags with just date_default_timezone_set and, from my POV, the current behaviour of \DateTime (ignoring an explicitly passed TZ) is faulty, so the effort here would workaround PHP's own deficiency.

Instead, we introduce a [KEEP/PRESERVE]_INPUT_TIMEZONE flag, set to true by default, which would only have effect on denormalization:

I feel this would again muddy the water here: if I'm explicitly specifying a timezone for the normalizer, why wouldn't it apply it when normalizing? The \Datetime instance could have arrived from many sources so it might be a chore on the developer to set it manually everywhere (maybe even recursively) before serialization.

The most clean behaviour to me seems like described:

• timezone not passed, use whatever is in input (denorm) or \DateTime instance (norm), falling back to whatever PHP thinks is best (by default TZ etc)
• timezone passed, normalize everything into the TZ provided by the developer

It has least possible use cases and things required to understand, least astonishment for the developer and no flags required. @ro0NL do you agree?

If you don't consider this a bug, then the way to get there is different and this would need a relabel.

My reasoning for this being a bug was this can be achieved without any context flags with just date_default_timezone_set

Yes and no. date_default_timezone_set is global, lower-level and will need to reset to previous timezone after. An application can handle multiple timezones, the default set in global might not be enough.

I feel this would again muddy the water here: if I'm explicitly specifying a timezone for the normalizer, why wouldn't it apply it when normalizing?

That'll become the default behavior in 5.0. So not an issue to me.
To me, it's more flexible and less hassle to contrib, as there will be no more flag to drop.

Yes and no. date_default_timezone_set is global, lower-level and will need to reset to previous timezone after. An application can handle multiple timezones, the default set in global might not be enough.

I understand that, but this train of thought seems like it allows one app to handle many timezones, but the lack of this feature (or from my POV, this bug) doesn't allow the app to use a single chosen timezone, it's optimizing for the more complex and less common use case. 😄

That'll become the default behavior in 5.0. So not an issue to me.
To me, it's more flexible and less hassle to contrib, as there will be no more flag to drop.

I'm afraid I don't understand what you mean here: since the behaviour for both norm/denorm would change in 5.0, this still needs to be done in 4.4 for the current behaviour to be deprecated, no? 🤔

The idea is to introduce the PRESERVE_INPUT_TIMEZONE flag:

• in 3.4 to 4.3, doesn't exist => current behavior is kept
• 4.4, PRESERVE_INPUT_TIMEZONE is allowed, but when not explicitly provided will behave as PRESERVE_INPUT_TIMEZONE => true, i.e: current behavior.
  The only difference is that a deprecation will be triggered when denormalizing without this flag:

   Using the `DateTimeNormalizer::TIMEZONE_KEY` without setting the `DateTimeNormalizer::PRESERVE_INPUT_TIMEZONE` to keep input timezone is deprecated. In 5.0, it'll default to `false` and will convert the timezone. Provide `DateTimeNormalizer::PRESERVE_INPUT_TIMEZONE => true` to keep the previous behavior.
  

• In 5.0: PRESERVE_INPUT_TIMEZONE is allowed, can be set to true or false, but defaults to false. So the behavior you expect becomes the default.

So the PR to get the expected behavior would just introduce the new flag in 4.4.
In 5.0, default value would be changed and deprecation dropped. And we're done. Both use-cases kept, no flag to deprecate & drop next. 😄

Thanks for all the feedback @ogizanagi , @ro0NL and @dkarlovi . I think that @ogizanagi idea is an elegant solution that allows for both BC and forward to what you originally requested @dkarlovi in your issue #31232 . Full integration will be delayed till 5.0, but of 4.4 the system will work both ways, with a deprecation thrown if the context flag is not set.
I will put some work into this soon.

Question: Should the final behaviour of the denormaliser include an override of the TIMEZONE_KEY with the php_ini timezone setting, rather than the default UTC for timestamps, or provided timezone detail in the time parameter when TIMEZONE_KEY is null?

My bad not correctly separating different PR's with appropriate branch names. Should I close both PR's and start again?

• PRESERVE_CONTEXT_TIMEZONE is a temporary key from NOW to version 5.0, where it is no longer required.
• Current code does NOT preserve the context timezone on denormalisation, but does on normalisation. This could be considered the bug, but some see it as correct behaviour.
• From now to 5.0:
  • not setting the PRESERVE_CONTEXT_TIMEZONE flag maintains the current behaviour. (denormalisation can ignore context timezone.) This preserves BC from now to 5.0.
  • setting a context timezone without setting PRESERVE_CONTEXT_TIMEZONE throws a deprecation to alert user to the upcoming change of behaviour, and the denormaliser uses PHP default behaviour.
  • set PRESERVE_CONTEXT_TIMEZONE to false to remove the deprecation warning, and the denormaliser uses PHP default behaviour.
  • set PRESERVE_CONTEXT_TIMEZONE to true to remove the deprecation warning, and denormalisation always applies the context timezone, unless context timezone is null.
• after 5.0:
  • this is BC break.
  • the PRESERVE_CONTEXT_TIMEZONE flag is ignored.
  • Denormalisation always applies the context timezone.
  • defaults to PHP behaviour if context timezone is null.

rebased to a single commit

Having new deprecations and new constants mean this should target master to me.

Indeed. Base merge for PR changed to master

I will be away till the 19th July.

Recommend Abandon these changes, as version in 5.1 has the required flexibility

Let’s close then. Thank you