Problem/Motivation

DateTimePlus wraps a protected \DateTime object and has magic methods for accessing the \DateTime object methods (proxy pattern).

Some of the \DateTime methods are meant to be chainable, but the __call() method does not allow this, they just returns the wrapped \DateTime object

Proposed resolution

Add a control within \Drupal\Component\Datetime\DateTimePlus::__call() magic method and If the proxied function call returns a DateTime object, then return the original DateTimePlus object, which allows function chaining to work properly.

Remaining tasks

User interface changes

None.

API changes

DateTimePlus (and by extension DrupalDateTime) methods modifying the current object now return the original object instead of the proxyed DateTime one and so they are now chain-able, i.e.:
Before:

$date = new \Drupal\Component\Datetime\DateTimePlus();
$date->add(new DateInterval('P10D'));// returns \DateTime
echo $date->render();

After:

$date = new \Drupal\Component\Datetime\DateTimePlus();
echo $date->add(new DateInterval('P10D'))->render();

Data model changes

None.

CommentFileSizeAuthor
#44 2910081-84x-44.patch6.15 KBmpdonadio
#36 interdiff-31-36.txt3.03 KBmpdonadio
#36 2910081-36.patch6.4 KBmpdonadio
#31 interdiff-24-31.txt720 bytesmpdonadio
#31 2910081-31.patch5.13 KBmpdonadio
#24 2910081-24.patch5 KBmpdonadio
#21 interdiff-15-21.txt3.36 KBmpdonadio
#21 2910081-21.patch4.75 KBmpdonadio
#15 interdiff.2910081-15.patch1.2 KBneclimdul
#15 datetimeplus_calls-2910081-15.patch3.21 KBneclimdul
#11 interdiff-08-11.txt982 bytesmpdonadio
#11 2910081-11.patch2.73 KBmpdonadio
#11 2910081-11-test-only.patch1.91 KBmpdonadio
#8 interdiff-04-08.txt1.08 KBmpdonadio
#8 2910081-08.patch2.86 KBmpdonadio
#8 2910081-08-test-only.patch2.04 KBmpdonadio
#4 2910081-04.patch1.79 KBmpdonadio
#2 2910081-02-test-only.patch1.02 KBmpdonadio
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

Status: Active » Needs review
FileSize
1.02 KB

Demo.

larowlan’s picture

/**
   * Implements the magic __call method.
   *
   * Passes through all unknown calls onto the DateTime object.
   */
  public function __call($method, $args) {
    // @todo consider using assert() as per https://www.drupal.org/node/2451793.
    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));
    }
    $return = call_user_func_array([$this->dateTimeObject, $method], $args);
    if ($return instanceof \DateTime) {
      $this->dateTimeObject = $return;
      return $this;
    }
    return $return;
  }
mpdonadio’s picture

Was thinking about get_class() because instanceof returns TRUE with subclasses.

The last submitted patch, 2: 2910081-02-test-only.patch, failed testing. View results

jhedstrom’s picture

Anything left to be done here? Fix looks reasonable to me, and the test demonstrates the issue nicely...

mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Needs review » Needs work

If we are OK with the approach (which it sounds like we are), I just want to add a little test coverage in DrupalDateTime to prevent regressions.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
2.04 KB
2.86 KB
1.08 KB

Blerg.

The last submitted patch, 8: 2910081-08-test-only.patch, failed testing. View results

jhedstrom’s picture

+++ b/core/tests/Drupal/Tests/Core/Datetime/DrupalDateTimeTest.php
@@ -156,4 +156,30 @@ public function providerTestInvalidDateDiff() {
+ * @returns string

Tiny nit: should be @return, (and theoretically have a description of the return?)

mpdonadio’s picture

In the end, this ends up doing the same thing as the shadowed global and still allows us to not have to set up mocks.

The last submitted patch, 11: 2910081-11-test-only.patch, failed testing. View results

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I think this is good to go!

neclimdul’s picture

Status: Reviewed & tested by the community » Needs work

How does this even pass? It doesn't locally.

+    $date->setTimestamp(12345678);
+    $rendered = $date->render();
+    $this->assertEquals('1970-05-24 07:21:18 Australia/Sydney', $rendered);
neclimdul’s picture

My bad, I see it hidden in bootstrap.php... I just ran phpunit directly for a quick test.

Pretty cool patch!

Explicity > Implied though because things break. So... I added a timezone to constructor similar to how the other tests are explicit.

I also assumed that __call wasn't being broken for things like diff that don't return date time but we don't have a test for that code path explicitly so I wrote it to confirm. Might as well add it to the test suite.

Also @covers help document what we're testing and the few of us running coverage checks can see what's going on better.

neclimdul’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: interdiff.2910081-15.patch, failed testing. View results

mpdonadio’s picture

[Crosspost].

The non-chainable coverage is good, and so is the @covers.

'Australia/Sydney' is the default timezone for tests. For unit tests, it is set in core/tests/bootstrap.php (see #2498619: Unit tests should use a default timezone other that UTC); the config is set in the various testbases. TestSuiteBaseTest, KernelTestBaseTest, and BrowserTestBaseTest all cover this assumption (see #2889803: Add test coverage for default time zone in tests). No harm in being explicit, though.

mpdonadio’s picture

Status: Needs work » Needs review

The last submitted patch, 15: datetimeplus_calls-2910081-15.patch, failed testing. View results

mpdonadio’s picture

This should pass now.

Also

- added an explicit time to prevent a small chance at a sporadic fail at second rollover
- may be overkill, but added test to make sure a chained call to a non-existent function does the right thing
- duped new changes into DrupalDateTimeTest

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

Failure tests? Be still my heart!

Thanks for fixing up my buggy changes.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2910081-21.patch, failed testing. View results

mpdonadio’s picture

Simple merge conflict in the tests.

gambry’s picture

Fix and test coverage look really good. I've added PHP 5.x tests just to make sure this works on all supported versions (no reasons why it shouldn't, but better safe than sorry). IMO this can be RTBC when tests are green.

gambry’s picture

Status: Needs review » Reviewed & tested by the community

As per #25.

gambry’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, just have some thoughts on this.

  1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
    @@ -314,7 +314,16 @@ public function __call($method, $args) {
    +
    +    if (is_object($val) && get_class($val) === 'DateTime') {
    

    We may want to say what we are doing in here? i.e. "Allow DateTimePlus to DateTime magic methods to still be chainable".

  2. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
    @@ -314,7 +314,16 @@ public function __call($method, $args) {
    +      $this->dateTimeObject = $val;
    

    Is this necessary? Isn't $this->dateTimeObject already updated at this point?
    (it is on my manual test).

neclimdul’s picture

2) I was thinking about that too. I think the idea is that the method could return a new date time object that is different in some way. I think if that where true we'd have a bit of another problem because we're actually mutating the object when the method we're proxying is creating a new object. Maybe what we do is something like compare $val to the internal object and if they are the same object do nothing, if they're different actually create a new DateTimePlus object. That said we might creating complexity for something that doesn't exist so we should review if its needed.

gambry’s picture

What DateTime method - DateTimePlus::__call() calls - would create another DateTime object?

The only methods __call() covers are:

public DateTime add ( DateInterval $interval )
public static array getLastErrors ( void )
public DateTime modify ( string $modify )
public DateTime setDate ( int $year , int $month , int $day )
public DateTime setISODate ( int $year , int $week [, int $day = 1 ] )
public DateTime setTime ( int $hour , int $minute [, int $second = 0 [, int $microseconds = 0 ]] )
public DateTime setTimestamp ( int $unixtimestamp )
public DateTime setTimezone ( DateTimeZone $timezone )
public DateTime sub ( DateInterval $interval )
public int getOffset ( void )
public int getTimestamp ( void )
public DateTimeZone getTimezone ( void )

And none of them does it. They all return either the current object or a completely different value, and we handle both scenarios.

neclimdul’s picture

Good to know. The proxy object has to exist for the cufa to succeed so we don't need the assignment or any complexity around the return type.

mpdonadio’s picture

Think the points in #27 are good to address.

gambry’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Yep. Thanks everyone!

Updating the IS.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
    @@ -315,7 +315,18 @@ public function __call($method, $args) {
    -    return call_user_func_array([$this->dateTimeObject, $method], $args);
    +
    +    $val = call_user_func_array([$this->dateTimeObject, $method], $args);
    +
    +    // If the proxied function call returns a DateTime object, then return
    +    // the original DateTimePlus object, which allows function chaining to
    +    // work properly.
    +    if (is_object($val) && get_class($val) === 'DateTime') {
    +      return $this;
    +    }
    +    else {
    +      return $val;
    +    }
       }
    

    This can be more elegant... looking at \GuzzleHttp\Psr7\StreamDecoratorTrait::__call()

        $result = call_user_func_array([$this->dateTimeObject, $method], $args);
        // If the proxied function call returns a DateTime object, then return
        // the original DateTimePlus object, which allows function chaining to
        // work properly.
        return $result === $this->dateTimeObject ? $this : $result;
    

    Also I think maybe we should move the documentation in a proper method docblock so the entire method would look something like:

      /**
       * Implements the magic __call method.
       *
       * Passes through all unknown calls onto the DateTime object.
       *
       * @param string $method
       *   The method to call on the decorated object.
       * @param array $args
       *   Call arguments.
       *
       * @return mixed
       *   The return value from the method on the decorated object. If the proxied
       *   method call returns a DateTime object, then return the original
       *   DateTimePlus object, which allows function chaining to work properly.
       * 
       * @throws \Exception
       *   Thrown when the DateTime object is not set.
       * @throws \BadMethodCallException
       *   Thrown when there is no corresponding method on the DateTime object to
       *   call.
       */
      public function __call($method, $args) {
        // @todo consider using assert() as per https://www.drupal.org/node/2451793.
        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));
        }
    
        $result = call_user_func_array([$this->dateTimeObject, $method], $args);
        return $result === $this->dateTimeObject ? $this : $result;
      }
    

    Equivalency of objects is super quick to check.

  2. Nice test coverage
  3. I think we need to do the same for \Drupal\Component\Datetime\DateTimePlus::__callStatic() since we have:
        /**
         * Parse a string into a new DateTime object according to the specified format
         * @param string $format Format accepted by date().
         * @param string $time String representing the time.
         * @param DateTimeZone $timezone A DateTimeZone object representing the desired time zone.
         * @return DateTime|boolean
         * @link http://php.net/manual/en/datetime.createfromformat.php
         */
        public static function createFromFormat ($format, $time, DateTimeZone $timezone=null) {}
    

    on DateTime...

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Good ideas. Re the change to `__callStatic()`, I'll add similar logic, but DTP overrides all of the current static methods on the \DT. That also means I am not sure if we can really have test coverage there; have to think about that some more.

gambry’s picture

As DateTimePlus already "overrides" all DateTime static methods. What would be the use of __callStatic() ?

mpdonadio’s picture

Here is #33 addressed. Minor tweak to the docblock suggestion. Read it several times and couldn't think of anything better.

Re the `__callStatic()` change, we already have a DateTimePlus::createFromFormat() as an explicit override. Nothing else on \DateTime needs this. Added some assertions around this. If a future version of PHP adds a new static method that returns a \DateTime, it would more than likely be a new factory method, and we would want a DTP specific one to match. So, I don't think we need to address this now.

?

alexpott’s picture

I missed that we were already overriding createFromFormat - so yeah that's sensible to not implement something that's not going to be used a present.

gambry’s picture

All #33 feedback looks been addressed by #36.
Manually tested and all still looks good, thanks!

neclimdul’s picture

Say what you will about ternary operators, I like that this patch is just boiled down to a ternary and a bunch of docs and tests. +1

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 601ba28 and pushed to 8.5.x. Thanks!

Unfortunately the patch does not apply to 8.4.x

I think this fix is eligible for 8.4.x as it is a BC compatible bug-fix. Whilst the return value of a method is changing the object returned has all the same capabilities. I'll leave it up to the sub-system maintainers to decide if they want to roll a patch for 8.4.x

  • alexpott committed 15c0299 on 8.5.x
    Issue #2910081 by mpdonadio, neclimdul, gambry, jhedstrom, alexpott:...
mpdonadio’s picture

Assigned: Unassigned » mpdonadio
Status: Fixed » Patch (to be ported)

Looks like the merge conflict is in the tests. Since they are new test methods, it should be an easy task.

While this may not have a big impact on DX directly, this was discovered as part of #2902707: Document magic methods in DateTimePlus and DrupalDateTime using phpDoc @method. Getting that into 8.4.x as a doc improvement as an "API documentation improvements" is a DX win since a lot of the common methods on DateTimePlus and DrupalDateTime get inherited from \DateTime, but can't autocomplete or typehint properly in IDEs. So, I think we should do an 8.4.x patch.

jhedstrom’s picture

I didn't know about the @property annotation for traits (seen in the psr7 class referenced above), so I opened #2917215: Adopt usage of the @property docblock annotation for traits since it really improves DX when editing/adding traits.

mpdonadio’s picture

Version: 8.5.x-dev » 8.4.x-dev
Assigned: mpdonadio » Unassigned
Status: Patch (to be ported) » Needs review
FileSize
6.15 KB

Here is a version against 8.4.x.

I took the patch from #36, remove the test hunks, applied, and then hand copy/pasted the new test methods in.

The merge conflict is b/c #2830094: Deprecate and remove usages of datetime_date_default_time(). is only in 8.5.x, so the hunk context in patch for the tests was incorrect, even though we are adding them to the end of the class.

Can't really do an interdiff, so someone needs to eyeball it.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

  • xjm committed a8896fb on 8.4.x
    Issue #2910081 by mpdonadio, neclimdul, gambry, jhedstrom, alexpott:...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

I tried to think of some way this might disrupt existing sites but couldn't really... this would satisfy any typehints that expected a DateTime(), support anything that was done to the previously returned DateTime, and if they're already using DateTimePlus then I couldn't come up with anything other than totally implausible scenarios where there might be method collisions.

So, committed to 8.4.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.