We need a new sniff for #2822837: Replace @expectedException @expectedExceptionMessage with $this->setExpectedException which I have just released.

CommentFileSizeAuthor
#6 2861793_6.patch13.01 KBmile23
#2 coder-2861793-2.patch12.91 KBklausi

Comments

klausi created an issue. See original summary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Needs review
StatusFileSize
new coder-2861793-2.patch12.91 KB

Patch.

There is a new detection for dots after @param names, which I have autofixed with phpcbf for this patch.

martin107’s picture

martin107 commented
Status: Needs review » Reviewed & tested by the community
Parent issue: » #2822837: Replace @expectedException @expectedExceptionMessage with $this->setExpectedException

I have made a visual scan of the patch.

all changes as per the coding standard
or changes to make the upgrade.

As mentioned, this is blocking another issue so I would like to see this go in as soon as possible.

klausi++

  • xjm committed c8cfd1d on 8.4.x 
    Issue #2861793 by klausi, martin107: Upgrade Coder to 8.2.12
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +rc eligible

Likey. Committed to 8.4.x.

As a patch-level update and a coding standards fix, this patch is RC eligible. What I'm confused about now is whether it should be cherry-picked. There used to be hashes that would differ, but apparently a recent composer update fixed that, or so klausi and dawehner explained while we were working on 8.2.7. So from now on would we just cherry-pick such eligible updates without needing a different patch?

In the case of this issue, though, different branches could have different violations for the new rules, so I guess it makes more sense to generate a new patch anyway.

So, PTBP for 8.3.x. Thanks!

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 2861793_6.patch13.01 KB

I just did the following:

$ git checkout 8.3.x
$ rm -rf vendor/
$ composer install
$ nano core/composer.json
# Modify coder to 8.2.12.
$ composer update drupal/coder
$ cd core
$ ../vendor/bin/phpcbf
$ ../vendor/bin/phpcs -ps
# No outstanding errors.
$ git diff 8.3.x > 2861793_6.patch
$ diff coder-2861793-2.patch 2861793_6.patch
# No differences other than index hashes.

So here's exactly the same patch. :-)

It doesn't apply to 8.2.x.

martin107’s picture

martin107 commented
Status: Needs review » Reviewed & tested by the community

Same as #3

visual scan - all the changes look good to me.

[ RTBC - Ahem assuming testbot comes back ok ]

Anonymous’s picture

Anonymous (not verified) commented

Yummy! We have already checked 8.4.x in #2822837-107: Replace @expectedException @expectedExceptionMessage with $this->setExpectedException, looks nice!

  • xjm committed 89d7032 on 8.3.x 
    Issue #2861793 by klausi, Mile23, martin107: Upgrade Coder to 8.2.12
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks @Mile23 for checking the patch. Committed and pushed to 8.3.x.

Status: Fixed » Closed (fixed)

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