Problem/Motivation

Update drupal/coder to the latest version.

Proposed resolution

Fix failing coding standards. Note DateTimePlus contains some non-standard documentation that doesn't work. See $settings documentation on https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Date... 

    * @param array $settings
    *   @see __construct()

The @see does not work at all - and doesn't work in PHPStorm either. Fortunately there are other methods that already have 

   * @param array $settings
   *   (optional) A keyed array for settings, suitable for passing on to
   *   __construct().

so we can copy that.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3099583-9.0.x-14.patch7.59 KBalexpott
#14 3099583-8.9.x-14.patch8.96 KBalexpott
#14 13-14-interdiff.txt888 bytesalexpott
#13 3099583-9.0.x-13.patch6.72 KBalexpott
#8 3099583-9.0.x-8.patch8.39 KBalexpott
#8 3099583-8.9.x-8.patch8.09 KBalexpott
#8 5-8-interdiff.txt577 bytesalexpott
#5 3099583-9.0.x-5.patch8.38 KBalexpott
#5 3099583-8.9.x-5.patch8.08 KBalexpott
#5 4-5-interdiff.txt765 bytesalexpott
#3 3099583-9.0.x-3.patch8.29 KBalexpott
#3 2-3-interdiff.txt368 bytesalexpott
#2 3099583-8.9.x-2.patch7.99 KBalexpott
#2 3099583-9.0.x-2.patch7.93 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Active » Needs review
FileSize
3099583-9.0.x-2.patch7.93 KB
3099583-8.9.x-2.patch7.99 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
2-3-interdiff.txt368 bytes
3099583-9.0.x-3.patch8.29 KB

There's one more fail on 9.0.x when running phpcs...

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Status: Needs review » Needs work

Looks good, just one super minor comment we should clean up:

+++ b/core/lib/Drupal/Component/Datetime/DateTimePlus.php
@@ -212,10 +215,13 @@ public static function createFromTimestamp($timestamp, $timezone = NULL, $settin
    * @param mixed $time
-   *   @see __construct()
+   *   String representing the time.

the data type says mixed but the comment says string. On the constructor the data type is also string. So let's change the data type to string here so that we don't introduce a contradicting comment.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review
FileSize
4-5-interdiff.txt765 bytes
3099583-8.9.x-5.patch8.08 KB
3099583-9.0.x-5.patch8.38 KB

Good idea. I borrowed the docs from core PHP

    /**
     * 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|false
     * @link https://php.net/manual/en/datetime.createfromformat.php
     */
    public static function createFromFormat ($format, $time, DateTimeZone $timezone=null) {}

So yep this has to be a string.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

PHPCS does not report problems, we should be good.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented 
+++ b/core/phpcs.xml.dist
@@ -329,6 +328,11 @@
+  <rule ref="Squiz.WhiteSpace.OperatorSpacing">
+  <properties>
+    <property name="ignoreNewlines" value="true"/>
+  </properties>
+  </rule>

The properties indention is off here. I guess we should nit the style on a style commit but that can be fixed on commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
5-8-interdiff.txt577 bytes
3099583-8.9.x-8.patch8.09 KB
3099583-9.0.x-8.patch8.39 KB

Fixed #7

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented

This needs backporting to 8.8. I'm using "drupal/core-dev": "^8.8" and right now my site is failing on running phpcs using the default core styles on 8.8.1.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: 3099583-9.0.x-8.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Annertech commented
Status: Needs work » Needs review

Patch for D8.9 in #8 applies cleanly and greenly on D8.8.x: https://www.drupal.org/pift-ci-job/1520837

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul at APQC commented
Status: Needs review » Reviewed & tested by the community

9 too. I think that might be some random or unrelated failure that happened before. LGTM.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3099583-9.0.x-13.patch6.72 KB

Okay so the 9.0.x branch has been updated - but is currently failing coder :( - here's a new patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
13-14-interdiff.txt888 bytes
3099583-8.9.x-14.patch8.96 KB
3099583-9.0.x-14.patch7.59 KB

Some CS errors have been added by recent patches...

  • catch committed a0bdf62 on 9.0.x 
    Issue #3099583 by alexpott, klausi, neclimdul, heddn: Update coder to 8....
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed a0bdf62 and pushed to 9.0.x, and cherry-picked to 8.9.x. Thanks!

  • catch committed 36c790e on 8.9.x 
    Issue #3099583 by alexpott, klausi, neclimdul, heddn: Update coder to 8....

Status: Fixed » Closed (fixed)

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