Fix and improve DateTimePlus documentation

This would be a duplicate of https://www.drupal.org/project/drupal/issues/2940420, but I was looking through the documentation for that class in more detail and there are a couple more documentation errors there. I think we can turn this one into a general fix for documentation errors/improvements for DateTimePlus?

Most of my assumptions are according to what's currently in https://www.drupal.org/node/1354, but I assume all of what's in there to be correct.

The class docblock

 * @method $this add(\DateInterval $interval)
 * @method static array getLastErrors()
 * @method $this modify(string $modify)
 * @method $this setDate(int $year, int $month, int $day)
 * @method $this setISODate(int $year, int $week, int $day = 1)
 * @method $this setTime(int $hour, int $minute, int $second = 0, int $microseconds = 0)
 * @method $this setTimestamp(int $unixtimestamp)
 * @method $this setTimezone(\DateTimeZone $timezone)
 * @method $this sub(\DateInterval $interval)
 * @method int getOffset()
 * @method int getTimestamp()
 * @method \DateTimeZone getTimezone()

Uses @method tags. While these are correct according to phpdoc, the Drupal API doesn't support them. The page displays these as a huge paragraph:

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

The @param $timezone docblocks:

   * @param mixed $timezone
   *   (optional) \DateTimeZone object, time zone string or NULL. NULL uses the
   *   default system time zone. Defaults to NULL.

Have a type of mixed. A more correct value would be, as the description states, \DateTimeZone|string|null

The @param $settings docblock on __construct():

   * @param array $settings
   *   (optional) Keyed array of settings. Defaults to empty array.
   *   - langcode: (optional) String two letter language code used to control
   *     the result of the format(). Defaults to NULL.
   *   - debug: (optional) Boolean choice to leave debug values in the
   *     date object for debugging purposes. Defaults to FALSE.

The only operation with the $settings parameter is the following:

    // Unpack settings.
    $this->langcode = !empty($settings['langcode']) ? $settings['langcode'] : NULL;

The 'debug' key is never used. It should be removed.
Also, according to the API documentation and comments standards:

The line before a list or before a new level of nesting must end with a colon (:).

The @param time docblock on createFromFormat():

   * @param mixed $time

The type is documented as mixed. It is actually a string, the same one accepted at https://www.php.net/manual/en/datetime.construct.php and the same type as most of the other $type parameters on that class.

Of course, multiple instances of @see tags as a child of the @param tag:

   * @param mixed $time
   *   @see __construct()
   * @param mixed $timezone
   *   @see __construct()

I don't think a @see __construct() is even needed in the first place. We can just copy the description of these parameters from the __construct() and paste it there. If we do need it we should link it at the very bottom of the docblock, as @see self::__construct() instead.
Note that the $settings param from createFromFormat() is the same as __construct(), with the exception that it can also contain a "validate_format" key. It seems to be the only one that is any different from the other ones.

Also some nitpicks:

  /**
   * A RFC7231 Compliant date.
   *
   * @see http://tools.ietf.org/html/rfc7231#section-7.1.1.1
   *
   * Example: Sun, 06 Nov 1994 08:49:37 GMT
   */
  const RFC7231 = 'D, d M Y H:i:s \G\M\T';

The description text should be after the short description text, and the @see tag should be at the bottom.

This patch should address the issues raised on #3. In addition to those, I added some @see tags (And regular See links, when using them inside @params) where I thought appropriate.

I had some trailing whitespaces. This one fixes those.

Tagging this for badcamp2019. (October 2-5)

Patch in #4 applies cleanly to 8.9.

I added the needs issue summary update tag because I think that it is a bit unclear what is left to be done on the issue. We should apply the standard issue summary template. THe issue summary should include the current state of the issue with respect to @joachim's comments and the latest patch.

Adding novice tag.

I'm working on this ticket at DrupalCon Amsterdam

updated issue summary to follow the standard issue summary template.

Changes have been made as per #8 and #9

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -24,18 +24,7 @@
  *
- * @method $this add(\DateInterval $interval)
- * @method static array getLastErrors()
- * @method $this modify(string $modify)
- * @method $this setDate(int $year, int $month, int $day)
- * @method $this setISODate(int $year, int $week, int $day = 1)
- * @method $this setTime(int $hour, int $minute, int $second = 0, int $microseconds = 0)
- * @method $this setTimestamp(int $unixtimestamp)
- * @method $this setTimezone(\DateTimeZone $timezone)
- * @method $this sub(\DateInterval $interval)
- * @method int getOffset()
- * @method int getTimestamp()
- * @method \DateTimeZone getTimezone()

These are all IDE hints. Unfortunate that Drupal API doesn't support them but removing them is a big DX hit so seems a no go.

Patch #14 Fails to Apply. Needs Reroll.

here the re-roll patch

This still needs to address the comment #16

This patch fixes #16

Addressing #16, I looked into it a bit more, and there is an issue to add the @method tags for the magic methods to the DateTimePlus docblock on #2902707: Document magic methods in DateTimePlus and DrupalDateTime using phpDoc @method. There's also an issue opened to support these on the Drupal Coding Standards API here #2920333: Allow PHPDoc @method annotations in class headers., which I agree with.

We should keep them and focus the attention on changing the Drupal Coding Standards API instead.

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

   Don't understand why these need space. also, trailing spaces.

2. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -46,9 +59,9 @@ class DateTimePlus {
   -   * @see http://tools.ietf.org/html/rfc7231#section-7.1.1.1
   -   *
       * Example: Sun, 06 Nov 1994 08:49:37 GMT
   +   *
   +   * @see http://tools.ietf.org/html/rfc7231#section-7.1.1.1
   

   not sure why we're moving this. Also maybe relevant to documenting this. #3177385: Deprecate DateTimePlugs::DATE_RFC7231 constant

3. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -195,6 +215,8 @@ public static function createFromArray(array $date_parts, $timezone = NULL, $set
   +   *
   +   * @see self::format()
       */
   

   What does format have to do with createFromTimestamp? Side note, I still don't understand why we can't use see comments per the phpdoc standard.

4. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -237,6 +259,9 @@ public static function createFromTimestamp($timestamp, $timezone = NULL, $settin
   +   * @see self::format()
   

   again, why format?

5. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -279,19 +304,20 @@ public static function createFromFormat($format, $time, $timezone = NULL, $setti
   +   *   (optional) Keyed array of settings. Defaults to empty array. Possible values include:
   
   @@ -677,12 +705,15 @@ public static function datePad($value, $size = 2) {
   +   *   (optional) Keyed array of settings. Defaults to empty array. Possible values include:
   

   >80 character comments.

6. +++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
   @@ -724,6 +755,8 @@ public function setDefaultDateTime() {
   +   *
   +   * @see https://www.php.net/manual/en/class.datetime.php
   

   Does this provide extra context? Isn't the return time clear?

Additional note, I think links to php.net documentation are suppose to remove the language from the path so it redirects for non-english speakers.

Updated the patch as suggested in #23.
- For point 3&4, the use of format() method is mentioned in the comment and I think that's why it is referenced.

/**
   * Creates a date object from timestamp input.
   *
   * The timezone of a timestamp is always UTC. The timezone for a
   * timestamp indicates the timezone used by the format() method.

- Not sure about point 2. still needs to be addressed.

Applied patch #24 and it is working fine.The problems addressed in #23 is fixed accordingly.

In #24 there is a comment that #23.2 needs to be addressed.

Also #23.6 needs to be answered.

I think this still needs and Issue Summary Update. The proposed resolution is simply a link to the API documentation standards which is a useful link but there is nothing to state what is exactly to be improved here. As a reviewer, it is impossible to know if the patch fixes the issue.

Updated patch for Drupal version 10.1.x

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

FYI #24 still cleanly applies to 10.1 so removing credit for #33

@kkalashnikov you can test by clicking retest.

Points in #28 still need to happen.

Addressed #23.2
Here's the updated patch. Please review.

FYI if you’re only going to address 1 point and not attempt the others the ticket is not ready for review

I think that in the state this issue is in it is no longer suitable for as a Novice issue. I am removing the 'Novice' tag because there isn't sufficient detail for someone new to Drupal contribution. Issues tagged Novice should met the criteria list in What makes a good novice task.