Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Yaml] Support parsing YAML timestamps as DateTime #14420

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

[Yaml] Support parsing YAML timestamps as DateTime #14420

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
238589d
Add $objectForMap as argument of Yaml::parse()
remi-blaise Apr 19, 2015
3daae64
[Yaml] Add an option to parse timestamps as DateTime
remi-blaise Apr 20, 2015
9db5c6f
[Yaml] Add DateTime support to the Dumper
remi-blaise Apr 20, 2015
663a9ca
[Yaml] Dump human-readable timestamps
remi-blaise Apr 20, 2015
41c5f12
[Yaml] Depreciate to not use DateTime
remi-blaise Apr 20, 2015
dd4798e
Worship fabbot.io
remi-blaise Apr 21, 2015
1f4269e
[Yaml] Accept DateTimeImmutable instances
remi-blaise May 2, 2015
028f2d7
[Yaml] Delete a cast
remi-blaise May 2, 2015
5d75dd0
[Yaml] Prepare merging in 2.8 rather than 2.7
remi-blaise May 2, 2015
72abdee
[Yaml] Rename $dateTimeSupport argument as $timestampAsDateTime
remi-blaise May 3, 2015
1b970f3
[Yaml] Fix deprecation
remi-blaise May 6, 2015
422d838
[Yaml] Fix usage of $timestampAsDateTime
remi-blaise May 6, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
[Yaml] Fix deprecation
  • Loading branch information
@remi-blaise
remi-blaise committed May 6, 2015
commit 1b970f3bdefae75b9a09a7ebe03ce1b627464fed
4 changes: 2 additions & 2 deletions 4 src/Symfony/Component/Yaml/Yaml.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class Yaml
*/
public static function parse($input, $exceptionOnInvalidType = false, $objectSupport = false, $objectForMap = false, $timestampAsDateTime = false)
{
if ($timestampAsDateTime) {
if (!$timestampAsDateTime) {
Copy link
Member

Choose a reason for hiding this comment

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

This triggers the deprecation message by default, which is bad

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but there is no other choice.

Copy link
Member

Choose a reason for hiding this comment

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

Well, if you cannot avoid triggering the warning for people using this method in the most common way (Yaml::parse($yaml)), you should not trigger it at all.
Currently, you will force everyone to pass all arguments explicitly to change the default to avoid the deprecation warning, which is a no-go

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think:

if (func_num_args() > 4) {
    // trigger_error(...);
}

this doesn't work if we add another param to the function, but it's something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dosten No bad idea. It's useless and it should be the same behaviour for everyone.

@stof So I remove the trigger but I keep the deprecation ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Stop What do you think ?

trigger_error('The ability to pass $timestampAsDateTime = false to the '.__METHOD__.' method is deprecated since version 2.8. The argument will be removed in 3.0. Pass true instead.', E_USER_DEPRECATED);
}

Expand Down Expand Up @@ -106,7 +106,7 @@ public static function parse($input, $exceptionOnInvalidType = false, $objectSup
*/
public static function dump($array, $inline = 2, $indent = 4, $exceptionOnInvalidType = false, $objectSupport = false, $timestampAsDateTime = false)
{
if ($timestampAsDateTime) {
if (!$timestampAsDateTime) {
trigger_error('The ability to pass $timestampAsDateTime = false to the '.__METHOD__.' method is deprecated since version 2.8. The argument will be removed in 3.0. Pass true instead.', E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

$timestampAsDateTime = false does not work anyway in your PR, so I suggest removing it entirely instead of adding a deprecated non-working behavior (and making it the default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes $timestampAsDateTime = false works.
$timestampAsDateTime is here for BC reasons. I have to give false by default. But it's not the advisable behavior so I put a deprecation. What's wrong ?

Copy link
Member

Choose a reason for hiding this comment

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

It does not work because your dumper does not care about the value passed here at all. Any time it finds a DateTimeInterface, it will dump it as a yaml timestamp, whatever the value of this flag is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah some times I feel idiot. I think for the BC reasons it's better to let the argument.
After it's not essential.

Copy link
Member

Choose a reason for hiding this comment

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

There is nothing about BC here. Your argument does strictly nothing as the code ignores it (except for the deprecation warning being triggered all the time by default, which is bad).
It should simply be removed

}

Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.