Skip to content
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

Jump to bottom

fix default value for logdateformat #3178

Merged
merged 2 commits into from
Jan 27, 2017
Merged

fix default value for logdateformat #3178

merged 2 commits into from
Jan 27, 2017

Conversation

MorrisJobke
Copy link
Member

cc @j-ed

@MorrisJobke
fix default value for logdateformat - fixes #3107
2316083 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke MorrisJobke added the 3. to review Waiting for reviews label Jan 20, 2017
@MorrisJobke MorrisJobke mentioned this pull request Jan 20, 2017
Closed
icewind1991
icewind1991 requested changes Jan 23, 2017
View reviewed changes
lib/private/Console/TimestampFormatter.php
@@ -101,7 +101,7 @@ public function format($message) {
$timeZone = $timeZone !== null ? new \DateTimeZone($timeZone) : null;

$time = new \DateTime('now', $timeZone);
$timestampInfo = $time->format($this->config->getSystemValue('logdateformat', 'c'));
$timestampInfo = $time->format($this->config->getSystemValue('logdateformat', \DateTime::ISO8601));
Copy link
Member

Choose a reason for hiding this comment

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

\DateTime::ISO8601 isn't actually ISO8601 (https://secure.php.net/manual/de/class.datetime.php#datetime.constants.iso8601), DateTime::ATOM is the one you're looking for

Copy link
Member

Choose a reason for hiding this comment

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

wow php, Y U not add @deprecated :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Sadly not what we currently log: https://secure.php.net/manual/en/function.date.php (the c also contains the colon) ... and I will for sure not change this format 🙈

Copy link
Member Author

Choose a reason for hiding this comment

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

@karlitschek I'm open for changing this to proper ISO8601 - we just need to make sure we put it into the release notes.

@MorrisJobke
add comment about ISO 8601 and PHP 🙈
a13ea67 
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@codecov-io
Copy link

Current coverage is 53.97% (diff: 66.66%)

Merging #3178 into master will increase coverage by 0.03%

@@             master      #3178   diff @@
==========================================
  Files          1299       1298     -1   
  Lines         80134      80110    -24   
  Methods        7909       7910     +1   
  Messages          0          0          
  Branches       1248       1248          
==========================================
+ Hits          43226      43240    +14   
+ Misses        36908      36870    -38   
  Partials          0          0
Diff Coverage File Path
0% lib/private/Console/TimestampFormatter.php
•••••••••• 100% config/config.sample.php
•••••••••• 100% lib/private/Log/File.php

Powered by Codecov. Last update 48a3f2a...a13ea67

@icewind1991
Copy link
Member

Can we use actual ISO8601 instead of the almost ISO8601 with the notice?

@MorrisJobke
Copy link
Member Author

Can we use actual ISO8601 instead of the almost ISO8601 with the notice?

But then we break all (log management) setups that rely on the fact that our log date contains the : 😉

@MorrisJobke
Copy link
Member Author

@icewind1991 can we get this in and decide on the log time format afterwards? Thanks :)

@nickvergessen nickvergessen merged commit 05884bc into master Jan 27, 2017
@nickvergessen nickvergessen deleted the default-value-logdateformat branch January 27, 2017 11:09
@nickvergessen
Copy link
Member

Actually you now changed the format: https://3v4l.org/MRe6f

var_dump(date('c'));
var_dump(date(\DateTime::ISO8601));
var_dump(date(\DateTime::ATOM));
---
string(25) "2017-01-30T15:00:45+01:00"
string(24) "2017-01-30T15:00:45+0100"
string(25) "2017-01-30T15:00:45+01:00"

So I will adjust this to ATOM

@nickvergessen nickvergessen mentioned this pull request Jan 30, 2017
Merged
@MorrisJobke
Copy link
Member Author

Actually you now changed the format:

I'm now totally confused ... -.- the PHP doc says something different.

@MorrisJobke
Copy link
Member Author

I'm now totally confused ... -.- the PHP doc says something different.

Ah okay - got it.

@MorrisJobke
Copy link
Member Author

Ah okay - got it.

I compared the new format not to the new format but to itself 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@icewind1991 icewind1991 icewind1991 approved these changes

@rullzer rullzer Awaiting requested review from rullzer

@LukasReschke LukasReschke Awaiting requested review from LukasReschke

@schiessle schiessle Awaiting requested review from schiessle

@blizzz blizzz Awaiting requested review from blizzz

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

different default values are used for parameter 'logdateformat`
4 participants
@MorrisJobke @codecov-io @icewind1991 @nickvergessen