Skip to content

Conversation

@paulbalandan
Copy link
Member

@paulbalandan paulbalandan commented Sep 3, 2022

Description
This might need discussion.

$ vendor/bin/php-cs-fixer describe date_time_create_from_format_call
Description of date_time_create_from_format_call rule.
The first argument of `DateTime::createFromFormat` method must start with `!`.
Consider this code:
    `DateTime::createFromFormat('Y-m-d', '2022-02-11')`.
    What value will be returned? '2022-01-11 00:00:00.0'? No, actual return value has 'H:i:s' section like '2022-02-11 16:55:37.0'.        
    Change 'Y-m-d' to '!Y-m-d', return value will be '2022-01-11 00:00:00.0'.
    So, adding `!` to format string will make return value more intuitive.

Fixer applying this rule is risky.
Risky when depending on the actual timings being used even when not explicit set in format.

Fixing examples:
 * Example #1.
   ---------- begin diff ----------
   --- Original
   +++ New
   @@ -1,1 +1,1 @@
   -<?php \DateTime::createFromFormat('Y-m-d', '2022-02-11');
   +<?php \DateTime::createFromFormat('!Y-m-d', '2022-02-11');

   ----------- end diff -----------

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide


$response->setLastModified(date('D, d M Y H:i:s'));
$response->setLastModified(DateTime::createFromFormat('u', $time));
$response->setLastModified(DateTime::createFromFormat('!u', $time));
Copy link
Member

@kenjis kenjis Sep 4, 2022

Choose a reason for hiding this comment

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

https://codeigniter4.github.io/CodeIgniter4/outgoing/response.html#CodeIgniter\HTTP\Response::setLastModified

This change is incorrect.

php > var_dump(DateTime::createFromFormat('u', 654321));
php shell code:1:
class DateTime#1 (3) {
  public $date =>
  string(26) "2022-09-04 00:00:00.654321"
  public $timezone_type =>
  int(3)
  public $timezone =>
  string(3) "UTC"
}
php > var_dump(DateTime::createFromFormat('!u', 654321));
php shell code:1:
class DateTime#1 (3) {
  public $date =>
  string(26) "1970-01-01 00:00:00.654321"
  public $timezone_type =>
  int(3)
  public $timezone =>
  string(3) "UTC"
}

Copy link
Member

Choose a reason for hiding this comment

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

The sample code doesn't make sense to begin with.

Copy link
Member Author

Choose a reason for hiding this comment

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

Based on the docs, this is expected.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Neither of the following seems to have a use case. What do we specify by $time?

$response->setLastModified(DateTime::createFromFormat('u', $time));
$response->setLastModified(DateTime::createFromFormat('!u', $time));

Copy link
Member Author

Choose a reason for hiding this comment

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

Resets all fields (year, month, day, hour, minute, second, fraction and timezone information) to zero-like values ( 0 for hour, minute, second and fraction, 1 for month and day, 1970 for year and UTC for timezone information)

This means that since the format lacks any of "Y, m, d, H, i, s", the ! resets them.

I think the format should have been the capital U.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're correct. u generates only the microseconds segment, so: 2022-09-10 07:37:24.5238 yields 5238. This was supposed to be U for an actual full timestamp.

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 no microseconds in the Last-Modified header:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Last-Modified

The sample code should be like the following:

$response->setLastModified(DateTime::createFromFormat('U', $timestamp));

And it is the same as:

$response->setLastModified(DateTime::createFromFormat('!U', $timestamp));

Copy link
Member

Choose a reason for hiding this comment

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

I sent a PR #6526

Copy link
Member

Choose a reason for hiding this comment

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

I think by definition !U is redundant.


$date = DateTime::createFromFormat('j-M-Y', '15-Feb-2016');
$date = DateTime::createFromFormat('!j-M-Y', '15-Feb-2016');
$response->setDate($date);
Copy link
Member

Choose a reason for hiding this comment

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

I do not know in which case such a sample code would be used.
https://codeigniter4.github.io/CodeIgniter4/outgoing/response.html#CodeIgniter\HTTP\Response::setDate
So I also don't know the change is correct or not.

@MGatner
Copy link
Member

MGatner commented Sep 4, 2022

Skipping this one for now, looks like it may need some more work.

@paulbalandan paulbalandan force-pushed the date-time-create-from-format-call branch from c893ec3 to ddf5b3e Compare September 7, 2022 03:45
@paulbalandan paulbalandan changed the title Enable date_time_create_from_format_call Disable date_time_create_from_format_call Sep 7, 2022
@paulbalandan
Copy link
Member Author

I disabled it for now.

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

I think that enabling date_time_create_from_format_call and fixing existing sample code that I can't imagine use cases might be better.

@MGatner
Copy link
Member

MGatner commented Sep 7, 2022

I can't find documentation on ! as a format modifier. Anyone have a link? It's not mentioned on https://www.php.net/manual/en/datetime.format.php

@paulbalandan
Copy link
Member Author

@MGatner
Copy link
Member

MGatner commented Sep 8, 2022

Ah thanks! It's specific to this method, makes sense. Anyone else bothered by this in that link?

ALl fields are initialised

Yes I think this is a good change to apply but it will require taking them case by case. I will start going through as I have time.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

Disabling for now is fine but please set this to true in the Coding Standard.

@paulbalandan paulbalandan force-pushed the date-time-create-from-format-call branch from ddf5b3e to 046e77f Compare September 9, 2022 02:45
@paulbalandan paulbalandan merged commit 1bfe531 into codeigniter4:develop Sep 9, 2022
@paulbalandan paulbalandan deleted the date-time-create-from-format-call branch September 9, 2022 03:00
paulbalandan added a commit to CodeIgniter/coding-standard that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants