DateTimePlus violates substitution principle of DateTime. Make it support PHP 7

This method is from http://php.net/manual/en/datetimeimmutable.createfromformat.php that we need for php 5.5 as well

Anyway we need to extend DataTime http://php.net/manual/en/datetime.createfromformat.php

EDIT The issue added this is #1844956: Optimize date formatting performance

So maybe better to rename the method because $timezone should be DateTimeZone type

Let's see if we still need the settings to pass, otoh we can loose ability to pass language

Would we have to conditionally define this method based on PHP version as per this?

I think over in the issue linked in #8 they are suggesting we do this... Though maybe we can push to have that commit reverted from PHP7 so we don't have to resort to patches like this? :)

I don't think that makes sense, then we'd provide a different API depending on the version. We can't do that.

It's not just the type hint, $timezone is a string in our implementation.

I see two options:

a) Rename the method. We didn't design it as an override, it might work differently than the parent.

b) Keep it, adjust the type hints and arguments, and check what the parent is doing exactly in 5.5, so that it is consistent to the native behavior, if possible.

Either way, we need to change the API.

Which makes this a release blocker, so changing to critical.

@Berdir yes more work would be needed than just that patch, I wasn't suggesting a different API for both versions! I asked about this because not having the type hint will give a strict warning on PHP7, and having it will give a strict warning on PHP5.5 (see the red tests in #4 and http://3v4l.org/DCOJB).

In any case, renaming the method seems like the better solution. Moreso as this was not intended to be an override, @andypost already suggested simply renaming in #3 as well.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -182,7 +182,7 @@ public static function createFromTimestamp($timestamp, $timezone = NULL, $settin
-  public static function createFromFormat($format, $time, $timezone = NULL, $settings = array()) {
+  public static function createFromFormatPlus($format, $time, $timezone = NULL, $settings = array()) {

Should we maybe encode the difference to createFromFormat in its name? What about choosing createFromFormatWithSettings ?

To be clear, I was just trying to make a suggestion, do you like it?

Yes, essentially it just calls DateTime::createFromFormat() using a settings array so I do think that's a clearer title :)

As the lower part of the patch shows, we have test coverage for that method, ... so I think this is RTBC.

Maybe someone will come, who disagrees with the name, but well, this solution is pragmatic.

i think we shouldn't we extending the class here - it makes us very vulnerable to changes in PHP's DateTime object. We should wrap it instead and provide a __call(). That way we can do this change with no API change.

Not sure how much refactoring we should be doing here, but lets see if we can find a way that makes sense.

@xjm: This is critical because this is otherwise giving strict warnings everywhere, and changing this somehow is needed to get rid of it. See #2454439-15: [META] Support PHP 7

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -251,7 +256,7 @@ public function __construct($time = 'now', $timezone = NULL, $settings = array()
-        parent::__construct($prepared_time, $prepared_timezone);
+        \DateTime::__construct($prepared_time, $prepared_timezone);

@@ -277,6 +282,16 @@ public function __toString() {
+    if (!isset($this->dateTimeObject)) {
+      $this->dateTimeObject = new \DateTime;
+    }

@@ -541,7 +556,7 @@ public function format($format, $settings = array()) {
+      $value = \DateTime::format($format);

I think you should store the wrapper object, otherwise it doesn't inheri the time / timezone

Updated issue summary now that tests are green

1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -92,6 +92,11 @@ class DateTimePlus extends \DateTime {
      /**
   +   * The DateTime object.
   +   */
   +  protected $dateTimeObject = NULL;
   

   Missing typehint

2. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -277,6 +282,22 @@ public function __toString() {
   +  public function __call($method, $args) {
   +    return call_user_func_array(array($this->dateTimeObject, $method), $args);
   +  }
   

   This needs to check if a dateTimeObject is set

3. +++ b/core/tests/Drupal/Tests/Component/Datetime/DateTimePlusTest.php
   @@ -238,7 +238,7 @@ public function testDateTimezone($input, $timezone, $expected_timezone, $message
   -    $input = new DateTimePlus('now', 'Pacific/Midway');
   +    $input = new \DateTime('now', new \DateTimeZone('Pacific/Midway'));
   

   I don't understand this change. I think this shows we need to implement a DateTimePlus::__construct that populates the dateTimeObject property.

@alexpott I don't understand your suggestion in #3, can you explain further? What do you mean by "populating", isn't $this->dateTimeObject already being set and populated with the time and the timezone in the constructor in the current patch? I.e. now that we do this:

- parent::__construct($prepared_time, $prepared_timezone);
+ $this->dateTimeObject = new \DateTime($prepared_time, $prepared_timezone);

As to the change in #3, I'm probably explaining the obvious and missing the point, but: the problem there is that DateTimePlus::createFromDateTime() requires a DateTime as its first argument per its type hint. Previously, as a DateTimePlus object, $input was a DateTime as well. This is not the case anymore now that we wrap the DateTimePlus class.

This needs to check if a dateTimeObject is set

Well, ideally this would be an assertion ... the constructor sets it, doesn't it?

As to the change in #3, I'm probably explaining the obvious and missing the point, but: the problem there is that DateTimePlus::createFromDateTime() requires a DateTime as its first argument per its type hint. Previously, as a DateTimePlus object, $input was a DateTime as well. This is not the case anymore now that we wrap the DateTimePlus class.

So what you can do is that DateTimePlus::createFromDateTime() converts the Datetime object passed in, into a DatteTimePlus object and then carry on.

Well, ideally this would be an assertion ... the constructor sets it, doesn't it?

Well the constructor sets the object already, but if there's an error intializing the object (ie. when doing DateTimePlus('invalid')), the exception that DateTime throws will be caught and the object will not be set.

So what you can do is that DateTimePlus::createFromDateTime() converts the Datetime object passed in, into a DateTimePlus object and then carry on.

It already does that :)

It already does that :)

A got it, thanks for the explanation!

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -277,6 +282,22 @@ public function __toString() {
   /**
+   * Passes through all unknown calls onto the DateTime object.
+   */
...
+  /**
+   * PHP magic __clone() method.
+   *
+   * Deep-clones the DateTime object we're wrapping.
+   */

Nitpick: I guess we can use {@inheritdoc} here.

Does this make sense?

OK patch is green again. The red "patch" was supposed to be a .txt file :)

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -277,6 +284,36 @@ public function __toString() {
+    if (!isset($this->dateTimeObject)) {
+      throw new \Exception('DateTime object not set.');
+    }
+    if (!method_exists($this->dateTimeObject, $method)) {
+      throw new \BadMethodCallException(sprintf('Call to undefined method %s::%s()', get_class($this), $method));
+    }
...
+    if (!method_exists('\DateTime', $method)) {
+      throw new \BadMethodCallException(sprintf('Call to undefined method %s::%s()', get_called_class(), $method));
+    }

Can we add a todo for pointing to #2451793: [META] Assert Statement Use in Drupal as those calls should be using an assertion in the future.

this adds a @todo as per #49

Thank you!

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -277,6 +284,37 @@ public function __toString() {
   /**
+   * {@inheritdoc}
+   */
...
+  /**
+   * {@inheritdoc}
+   */
...
+  /**
+   * {@inheritdoc}
+   */

Actually there's no docs to inherit here. So we need to provide them.

Actually there's no docs to inherit here. So we need to provide them.

Oh, you are right, its storm which is showing them.

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -284,7 +284,7 @@
   /**
-   * {@inheritdoc}
+   * Passes through all unknown calls onto the DateTime object.
    */

@@ -298,7 +298,7 @@
 
   /**
-   * {@inheritdoc}
+   * Passes through all unknown static calls onto the DateTime object.

@@ -308,7 +308,7 @@
   /**
-   * {@inheritdoc}
+   * Deep-clones the DateTime object we're wrapping.
    */

Note: We use the following bits in other parts of the code:

* Implements the magic __clone method.

We don't do it for __call and __callStatic elsewhere but since you listed those may as well do it here.

Sorry for the nitpicks!

Back to Alex, but looks good to me.

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0d0ed66 and pushed to 8.0.x. Thanks!