Document magic methods in DateTimePlus and DrupalDateTime using phpDoc @method

I'm in favor of this; I find magic methods very annoying in an PhpStorm. Do we do this anywhere else in core? Unfortunately \DateTime doesn't implement an interface, so we would be duplicating that and have the potential for getting out of sync.

Picking

Er, just re-read #2. \DateTime implements \DateTimeInterface, but only really as a way to have interoperability with the \DateTimeImmutable version. Many of the common functions used aren't on the interface.

Documented all methods of DateTime.
Refernce: http://php.net/manual/en/class.datetime.php

Kindly review. Thanks !

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -17,6 +17,23 @@
+ * @method \DateTime add ( DateInterval $interval )
...
+ * @method \DateTimeZone getTimezone ( void )

Although the @method tag is new to the Drupal standars for code comments I think we should still follow the consistent syntax for declaring/calling functions. From the documentation for Function Calls:

no spaces between the function name, the opening parenthesis, and the first parameter; spaces between commas and each parameter, and no space between the last parameter, the closing parenthesis, and the semicolon.

Having a closer look at #5 patch and most @method have errors, i.e.:

1. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -17,6 +17,23 @@
   + * @method \DateTime add ( DateInterval $interval )
   

   Undefined class DateInterval. Should that be \DateInterval?

2. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -17,6 +17,23 @@
   + * @method static \DateTime createFromFormat ( string $format , string $time [, DateTimeZone $timezone ] )
   

   Declaration should be compatible with DateTimePlus::createFromFormat().

Etc.

@gambry I tried fixed but I found with createFromFormat and I don't know if I could do with this way, -> with parameters

I tried fixed but I found with createFromFormat and I don't know if I could do with this way, -> with parameters

Not sure I understand this comment. The method declaration now looks to be compatible with DateTimePlus::createFromFormat.

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -17,6 +17,25 @@
+ * @method \DateTime add(\DateInterval $interval)
+ * @method static \DateTime createFromFormat($format, $time,  $timezone = NULL, $settings = [])
+ * @method static array getLastErrors()
+ * @method \DateTime modify(string $modify)
+ * @method static \DateTime __set_state(array $array)
+ * @method \DateTime setDate(int $year, int $month, int $day)
+ * @method \DateTime setISODate(int $year, int $week, int $day = 1)
+ * @method \DateTime setTime(int $hour, int $minute, int $second = 0, int $microseconds = 0)
+ * @method \DateTime setTimestamp(int $unixtimestamp)
+ * @method \DateTime setTimezone(\DateTimeZone $timezone)
+ * @method \DateTime sub(\DateInterval $interval)
+ * @method \DateInterval diff($datetime2, $absolute = FALSE)
+ * @method int getOffset()
+ * @method int getTimestamp()
+ * @method \DateTimeZone getTimezone()

I think what #7 was getting at is that all of these should be declared to return DateTimePlus and be added to that class and not DrupalDateTime. Yes, these operate on the protected object, but this should not "downgrade" the class of the object being acted on.

@ methods now return DateTimePlus rather than \DateTime.

What I meant on #7 was most methods definitions on #6 were flagged as wrong by my IDE, due coding standard issues and incompatibility with DateTimePlus definitions, i.e. createFromFormat().

all of these should be declared to return DateTimePlus

I don't think that's correct. Magic __call() returned items are actually still DateTime objects, aren't they?
In this case we don't want to let IDEs and developers think those are DateTimePlus when they are actually not.

What I think we should do in here is:

1. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -17,6 +17,25 @@
   + * @param string $format
   + * @param string $time
   

   These shouldn't be in the class docblock. The __constructor already takes care of them.

2. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -17,6 +17,25 @@
   + * @method DateTimePlus add(\DateInterval $interval)
   ...
   + * @method DateTimePlus modify(string $modify)
   + * @method static DateTimePlus __set_state(array $array)
   + * @method DateTimePlus setDate(int $year, int $month, int $day)
   + * @method DateTimePlus setISODate(int $year, int $week, int $day = 1)
   + * @method DateTimePlus setTime(int $hour, int $minute, int $second = 0, int $microseconds = 0)
   + * @method DateTimePlus setTimestamp(int $unixtimestamp)
   + * @method DateTimePlus setTimezone(\DateTimeZone $timezone)
   + * @method DateTimePlus sub(\DateInterval $interval)
   

   These should return \DateTime.

3. +++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
   @@ -17,6 +17,25 @@
   + * @method static DateTimePlus createFromFormat($format, $time, $timezone = NULL, $settings = [])
   

   This can be removed. DateTimePlus has its own definition for this, without relying on the DateTime one through the __call() magic method.

Then we can refactor DateTimePlus::__call() method to actually return DateTimePlus object instead of DateTime and updating affected phpDoc @methods defined in here. In alternative we can postpone this until the refactoring is done.

Thoughts?

1. Removed @params.
2. @methods once again return\DateTime rather than DateTimePlus.
3. Removed @method static DateTimePlus createFromFormat($format, $time, $timezone = NULL, $settings = []).

Can you explain what you mean about refactoring DateTimePlus::__call(), please?

Hey @Jo Fitzgerald , thank for the changes. I haven't moved the issue status to Needs work as #12 was more a question for @mpdonadio and @jhedstrom than a real request.

Can you explain what you mean about refactoring DateTimePlus::__call(), please?

At the moment methods listed in #12.2 return a DateTime object. IMHO if we want to do what #10 suggests then those method should return DateTimePlus, so a bit of refactoring is needed.

I think we are good with your work on #13. I'd leave the final decision about which is the right Class to suggest through @method(s) and about refactoring to the datetime maintainers.

I'm not going to have time to play with this for few days. I'm worried about BC if we refactor these, but I don't think I put 2-and-2 together about the return types and this could explain some weirdness I have seen chaining calls. Have to think on this...

+++ b/core/lib/Drupal/Core/Datetime/DrupalDateTime.php
@@ -17,6 +17,21 @@
+ * @method \DateInterval diff($datetime2, $absolute = FALSE)

`diff` is already implemented as DateTimePlus::diff(), so this can go.

The conundrum is if you look at the docs, eg http://php.net/manual/en/datetime.settimestamp.php

Returns the DateTime object for method chaining or FALSE on failure.

So, these are supposed to be chainable, but the implementation doesn't allow it; the wrapped object gets returned. Blerg.

Just made #2910081: DateTimePlus calls should be chainable, but I think this may worse than I thought...

Postponing on #2910081: DateTimePlus calls should be chainable.

Unpostponing since #2910081: DateTimePlus calls should be chainable is in both 8.5.x and backported to 8.4.x.

Keeping this at 8.5.x, but should be 8.4.x eligible as an "API documentation improvements".

So, lets wrap this up.

Feedback in #16 needs to be addressed, and we should update both DateTimePlus and DrupalDateTime to reflect that the proper object type is returned, and not \DateTime now.

Decided to remove __set_state(), since that is really just used by var_dump().

PhpStorm complains about the changes to DrupalDateTime, but looks like everything works as expected.

I manually tested with these, and as expected, method autocompletion works with this patch, and doesn't without it.

PhpStorm complains about the changes to DrupalDateTime

It seemed to work fine. What was the complaint?

This is RTBC assuming that isn't an issue.

PhpStorm 2017.2.4 says this:

I don't see it as a show stopper, just that the inspection isn't smart enough to fully understand the magic __call w/o us redeclaring it.

I get the same errors as @mpdonadio and the reason is clearly those methods are actually already part of DrupalDateTime and PHPStorm doesn't understand why we are trying to override their return types.
But if we remove the @method tags from DrupalDateTime then the IDE thinks returned values are DateTimePlus, which is not the end of the world but still technically wrong.

A solution seems to be using $this, as explained here. It works on PHPStorm but I can't understand if it's part of the tag definition or it-just-magically-works.

Attached a patch using $this as returned type.

There is a PHP Standards Recommendation (PSR-5) currently in draft proposing $this as return type.

$this, the element to which this type applies is the same exact instance as the current class in the given context. As such this type is a stricter version of static as, in addition, the returned instance must not only be of the same class but also the same instance.

The most popular IDE already uses it.

The patch in #23 is missing the changes to DrupalDateTime, but locally changing those to also use $this resolves the php storm complaint.

@jhedstrom as DrupalDateTime extends DateTimePlus, if we use $this as @method return type in DateTimePlus we don't need to patch DrupalDateTime.

@gambry ah yes, I missed that bit. It does indeed work with the patch in #23.

So regarding #24, can we use $this before it's 'official'?

`@return $this` is used in core already for chainable things, so this (ha!) should be OK for `@method`. See EntityPublishedInterface for an example.

I think this is good to go then.

Huh interesting. This isn't a phpdoc pattern we have yet anywhere in core:
grep -r "* @method" * | grep "php" | grep -v "vendor"

I found the canonical reference here: https://docs.phpdoc.org/references/phpdoc/tags/method.html

It is used in a few vendor libraries:

[ibnsina:maintainer | Wed 14:00:07] $ grep -rl "* @method" * | grep "php" | grep "vendor"
vendor/guzzlehttp/guzzle/src/Client.php
vendor/phpspec/prophecy/spec/Prophecy/Doubler/ClassPatch/MagicCallPatchSpec.php
vendor/phpunit/phpunit-mock-objects/src/Framework/MockObject/MockObject.php
vendor/symfony/http-kernel/Client.php

I think what's in the patch looks in line with what we already have for other tags like @param, @return, @throws, etc.: Tag, followed by datatype, followed by thing; no blank lines between the items; blank lines between the section containing that doc tag and other tag sections.

Before we commit this as a one-off, let's file a followup issue in the coding standards queue: https://www.drupal.org/project/coding_standards

I don't think we need to block committing this on that being fully adopted, but since this is the first example for core, let's at least get the issue filed so any! followup discussion can go in that standards issue. Once it's filed this can probably be RTBC again.

Thanks!

Created #2920333: Allow PHPDoc @method annotations in class headers..

Thanks @mpdonadio.

Committed to 8.5.x. We're in commit freeze right now since 8.4.1 just landed a couple hours ago, so leaving this RTBC against 8.4.x for backport.

Backported to 8.4.x as well now. Thanks everyone.