Problem/Motivation

Since PHP 8.0 we can omit the exception variable in catch blocks if the variable is unused.

Steps to reproduce

Proposed resolution

Add SlevomatCodingStandard.Exceptions.RequireNonCapturingCatch to phpcs.xml.dist and run composer phpcbf.

Remaining tasks

Discuss coding standards

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3421809

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

mstrelan created an issue. See original summary.

mstrelan opened merge request !6636

mstrelan’s picture

mstrelan commented
Status: Active » Needs review
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Needs work

Drupal\KernelTests\Core\Plugin\DefaultPluginManagerTest disagrees ;)

mstrelan’s picture

mstrelan commented

So apparently this sniff doesn't detect use of the variable in finally clauses. That seems like a bug with the sniff. That said, it seems weird to access the exception from the finally clause, because it's not guaranteed to be set.

mstrelan’s picture

mstrelan commented
Status: Needs work » Postponed

Looked closer at the test. $e is set to NULL in the try clause and should become a Throwable in the catch, so it should definitely exist in finally.

I guess this is postponed on False positive SlevomatCodingStandard.Exceptions.RequireNonCapturingCatch.NonCapturingCatchRequired.

spokje’s picture

spokje
Primary language Dutch
commented
Title: Remove unused variables in catch statements » [PP-Upstream] Remove unused variables in catch statements

Thanks for the Sherlocking @mrstrelan!

mstrelan’s picture

mstrelan commented

Elementary!

mstrelan’s picture

mstrelan commented
Title: [PP-Upstream] Remove unused variables in catch statements » Remove unused variables in catch statements
Status: Postponed » Needs work

Issue mentioned in #6 is fixed now and released in slevomat/coding-standard:8.15.0

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Slevomat coding standard was updated in #3262874: Update Coder to 8.3.15. Rebased and re-ran phpcbf. Back to Needs Review.

spokje’s picture

spokje
Primary language Dutch
commented

I think you've beaten Moriarty @mstrelan!

Since this adds a PHPCS-rule and might break contrib/external-CI in some places, I think we need a CR for this.
Besides that, I'm all for RTBC.

mstrelan’s picture

mstrelan commented

Contrib doesn't use the phpcs file from core, but we could indeed add a CR regardless.

mstrelan’s picture

mstrelan commented

I don't see CR's for the latest 3 commits to phpcs.xml.dist, so maybe we're ok?

#2874067: Fix Drupal.Commenting.DocCommentLongArraySyntax coding standard
#3180696: Fix 'Drupal.Commenting.TodoComment' coding standard
#3427999: [PHP 8.4] Fix implicitly nullable type declarations

Also in #3180696-89: Fix 'Drupal.Commenting.TodoComment' coding standard @quietone mentioned CR is not needed.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs review » Reviewed & tested by the community

Thanks sherlock mstrelan, seems like it's indeed not needed.
RTBC for me.

  • catch committed d961c6dc on 11.x 
    Issue #3421809 by mstrelan, Spokje: Remove unused variables in catch...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Yes core's phpcs only affects core afaik, so we don't need a CR for these. Committed/pushed to 11.x, thanks!

catch closed merge request !6636

Status: Fixed » Closed (fixed)

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