Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: require non-capturing catch #281

Merged
merged 1 commit into from Jul 18, 2022
Merged

feat: require non-capturing catch #281

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/Doctrine/ruleset.xml
View file
Open in desktop
Expand Up @@ -287,6 +287,8 @@
<rule ref="SlevomatCodingStandard.Exceptions.DeadCatch"/>
<!-- Require using Throwable instead of Exception -->
<rule ref="SlevomatCodingStandard.Exceptions.ReferenceThrowableOnly"/>
<!-- Require non-capturing catch when the variable with exception is not used. -->
<rule ref="SlevomatCodingStandard.Exceptions.RequireNonCapturingCatch" />
<!-- Ensure Arrow Functions declaration format -->
<rule ref="SlevomatCodingStandard.Functions.ArrowFunctionDeclaration">
<properties>
Expand Down
5 changes: 5 additions & 0 deletions tests/fixed/Exceptions.php
View file
Open in desktop
@@ -0,0 +1,5 @@
<?php

declare(strict_types=1);

namespace Exceptions;
Comment on lines +1 to +5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these files supposed to have no body? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it's a base file and the body is added via 80 and 81 patches. No support in < 8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's just drop php 7

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree but not my call. Let's have @doctrine team do that.

Copy link
Member

@greg0ire greg0ire Jul 16, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, why not? We run the cs jobs on PHP 8, and can use ^9.0 || ^10.0 as a version constraint on projects that need to support PHP 7.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah but I get why you would be reluctant to do so: #226

@beberlei , if the code still works, but the tests are harder to write, can we maybe drop support for PHP 7?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repository does not contain any code, so I don't see a point in requiring a higher PHP version than the sum of our dependencies. @greg0ire is right that we don't run PHPCS itself on PHP 7 anymore, but we run it on code that needs to run on PHP 7.

What the test suite currently covers isn't running the ruleset on different PHP versions, but running it for different language levels. And we would still need that matrix, even if we bumped the min PHP version in composer.json to 8.2.

I understand that reviewing those patch files is cumbersome. Maybe it would be a little less painful if we created dedicated fixed folders per language level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be a little less painful if we created dedicated fixed folders per language level?

Inputs are also different for different PHP versions.

Copy link
Member

@derrabus derrabus Jul 18, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. And do you think it would help to reverse the patching process? We'd keep all fixtures in the highest supported PHP language level (8.1 currently) and create patches for the lower levels? My expectation would be that PRs would become easier to review and it would be easier to drop old PHP versions eventually.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintenance would still be PIA (since maintaining eoled phps sucks and there's too many versions already) but it may facilitate reviews, yes.

5 changes: 5 additions & 0 deletions tests/input/Exceptions.php
View file
Open in desktop
@@ -0,0 +1,5 @@
<?php

declare(strict_types=1);

namespace Exceptions;
69 changes: 64 additions & 5 deletions tests/php80-compatibility.patch
View file
Open in desktop
@@ -1,15 +1,16 @@
diff --git a/tests/expected_report.txt b/tests/expected_report.txt
index 1d5a7d3..03460f2 100644
index 53dada5..741d972 100644
--- a/tests/expected_report.txt
+++ b/tests/expected_report.txt
@@ -14,23 +14,24 @@ tests/input/constants-var.php 7 0
@@ -14,23 +14,25 @@ tests/input/constants-var.php 7 0
tests/input/ControlStructures.php 28 0
tests/input/doc-comment-spacing.php 11 0
tests/input/duplicate-assignment-variable.php 1 0
-tests/input/EarlyReturn.php 6 0
-tests/input/example-class.php 38 0
+tests/input/EarlyReturn.php 7 0
+tests/input/example-class.php 43 0
+tests/input/Exceptions.php 1 0
tests/input/forbidden-comments.php 14 0
tests/input/forbidden-functions.php 6 0
tests/input/inline_type_hint_assertions.php 7 0
Expand All @@ -35,7 +36,7 @@ index 1d5a7d3..03460f2 100644
tests/input/semicolon_spacing.php 3 0
tests/input/single-line-array-spacing.php 5 0
tests/input/spread-operator.php 6 0
@@ -39,16 +40,17 @@ tests/input/strings.php 1 0
@@ -39,16 +41,17 @@ tests/input/strings.php 1 0
tests/input/superfluous-naming.php 11 0
tests/input/test-case.php 8 0
tests/input/trailing_comma_on_array.php 1 0
Expand All @@ -50,13 +51,26 @@ index 1d5a7d3..03460f2 100644
+tests/input/UselessConditions.php 21 0
----------------------------------------------------------------------
-A TOTAL OF 382 ERRORS AND 0 WARNINGS WERE FOUND IN 42 FILES
+A TOTAL OF 421 ERRORS AND 0 WARNINGS WERE FOUND IN 44 FILES
+A TOTAL OF 422 ERRORS AND 0 WARNINGS WERE FOUND IN 45 FILES
----------------------------------------------------------------------
-PHPCBF CAN FIX 317 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
+PHPCBF CAN FIX 356 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
+PHPCBF CAN FIX 357 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


diff --git a/tests/fixed/ControlStructures.php b/tests/fixed/ControlStructures.php
index a653086..f8f7f65 100644
--- a/tests/fixed/ControlStructures.php
+++ b/tests/fixed/ControlStructures.php
@@ -104,7 +104,7 @@ class ControlStructures

try {
echo 4;
- } catch (Throwable $throwable) {
+ } catch (Throwable) {
}

echo 5;
diff --git a/tests/fixed/EarlyReturn.php b/tests/fixed/EarlyReturn.php
index caf1dbb..fc734db 100644
--- a/tests/fixed/EarlyReturn.php
Expand All @@ -70,6 +84,22 @@ index caf1dbb..fc734db 100644
{
foreach ($items as $item) {
if (! $item->isItem()) {
diff --git a/tests/fixed/Exceptions.php b/tests/fixed/Exceptions.php
index 9b146c6..db7408b 100644
--- a/tests/fixed/Exceptions.php
+++ b/tests/fixed/Exceptions.php
@@ -3,3 +3,11 @@
declare(strict_types=1);

namespace Exceptions;
+
+use Exception;
+use Throwable;
+
+try {
+ throw new Exception();
+} catch (Throwable) {
+}
diff --git a/tests/fixed/NamingCamelCase.php b/tests/fixed/NamingCamelCase.php
index 57d9f2b..5493471 100644
--- a/tests/fixed/NamingCamelCase.php
Expand Down Expand Up @@ -395,6 +425,35 @@ index 10e6f34..5e26ed8 100644
- private $x = 1;
+ private int|string|null $x = 1;
}
diff --git a/tests/input/ControlStructures.php b/tests/input/ControlStructures.php
index a0e0b2e..73944e3 100644
--- a/tests/input/ControlStructures.php
+++ b/tests/input/ControlStructures.php
@@ -93,7 +93,7 @@ class ControlStructures
}
try {
echo 4;
- } catch (Throwable $throwable) {
+ } catch (Throwable) {
}
echo 5;
}
diff --git a/tests/input/Exceptions.php b/tests/input/Exceptions.php
index 9b146c6..3aaa30f 100644
--- a/tests/input/Exceptions.php
+++ b/tests/input/Exceptions.php
@@ -3,3 +3,11 @@
declare(strict_types=1);

namespace Exceptions;
+
+use Exception;
+use Throwable;
+
+try {
+ throw new Exception();
+} catch (Throwable $throwable) {
+}
diff --git a/tests/input/PropertyDeclaration.php b/tests/input/PropertyDeclaration.php
index 0891e12..4eb8164 100644
--- a/tests/input/PropertyDeclaration.php
Expand Down
69 changes: 64 additions & 5 deletions tests/php81-compatibility.patch
View file
Open in desktop
@@ -1,8 +1,8 @@
diff --git a/tests/expected_report.txt b/tests/expected_report.txt
index 1d5a7d3..e9394b1 100644
index 53dada5..d53fd48 100644
--- a/tests/expected_report.txt
+++ b/tests/expected_report.txt
@@ -14,23 +14,25 @@ tests/input/constants-var.php 7 0
@@ -14,23 +14,26 @@ tests/input/constants-var.php 7 0
tests/input/ControlStructures.php 28 0
tests/input/doc-comment-spacing.php 11 0
tests/input/duplicate-assignment-variable.php 1 0
Expand All @@ -11,6 +11,7 @@ index 1d5a7d3..e9394b1 100644
+tests/input/EarlyReturn.php 7 0
+tests/input/example-class.php 43 0
+tests/input/ExampleBackedEnum.php 3 0
+tests/input/Exceptions.php 1 0
tests/input/forbidden-comments.php 14 0
tests/input/forbidden-functions.php 6 0
tests/input/inline_type_hint_assertions.php 7 0
Expand All @@ -36,7 +37,7 @@ index 1d5a7d3..e9394b1 100644
tests/input/semicolon_spacing.php 3 0
tests/input/single-line-array-spacing.php 5 0
tests/input/spread-operator.php 6 0
@@ -39,16 +41,17 @@ tests/input/strings.php 1 0
@@ -39,16 +42,17 @@ tests/input/strings.php 1 0
tests/input/superfluous-naming.php 11 0
tests/input/test-case.php 8 0
tests/input/trailing_comma_on_array.php 1 0
Expand All @@ -51,13 +52,26 @@ index 1d5a7d3..e9394b1 100644
+tests/input/UselessConditions.php 21 0
----------------------------------------------------------------------
-A TOTAL OF 382 ERRORS AND 0 WARNINGS WERE FOUND IN 42 FILES
+A TOTAL OF 427 ERRORS AND 0 WARNINGS WERE FOUND IN 45 FILES
+A TOTAL OF 428 ERRORS AND 0 WARNINGS WERE FOUND IN 46 FILES
----------------------------------------------------------------------
-PHPCBF CAN FIX 317 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
+PHPCBF CAN FIX 362 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
+PHPCBF CAN FIX 363 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


diff --git a/tests/fixed/ControlStructures.php b/tests/fixed/ControlStructures.php
index a653086..f8f7f65 100644
--- a/tests/fixed/ControlStructures.php
+++ b/tests/fixed/ControlStructures.php
@@ -104,7 +104,7 @@ class ControlStructures

try {
echo 4;
- } catch (Throwable $throwable) {
+ } catch (Throwable) {
}

echo 5;
diff --git a/tests/fixed/EarlyReturn.php b/tests/fixed/EarlyReturn.php
index caf1dbb..fc734db 100644
--- a/tests/fixed/EarlyReturn.php
Expand All @@ -83,6 +97,22 @@ index fe54eb9..cc38c54 100644
+enum ExampleBackedEnum: int
+{
+}
diff --git a/tests/fixed/Exceptions.php b/tests/fixed/Exceptions.php
index 9b146c6..db7408b 100644
--- a/tests/fixed/Exceptions.php
+++ b/tests/fixed/Exceptions.php
@@ -3,3 +3,11 @@
declare(strict_types=1);

namespace Exceptions;
+
+use Exception;
+use Throwable;
+
+try {
+ throw new Exception();
+} catch (Throwable) {
+}
diff --git a/tests/fixed/NamingCamelCase.php b/tests/fixed/NamingCamelCase.php
index 57d9f2b..5493471 100644
--- a/tests/fixed/NamingCamelCase.php
Expand Down Expand Up @@ -408,6 +438,19 @@ index 10e6f34..5e26ed8 100644
- private $x = 1;
+ private int|string|null $x = 1;
}
diff --git a/tests/input/ControlStructures.php b/tests/input/ControlStructures.php
index a0e0b2e..73944e3 100644
--- a/tests/input/ControlStructures.php
+++ b/tests/input/ControlStructures.php
@@ -93,7 +93,7 @@ class ControlStructures
}
try {
echo 4;
- } catch (Throwable $throwable) {
+ } catch (Throwable) {
}
echo 5;
}
diff --git a/tests/input/ExampleBackedEnum.php b/tests/input/ExampleBackedEnum.php
index fe54eb9..0c47286 100644
--- a/tests/input/ExampleBackedEnum.php
Expand All @@ -420,6 +463,22 @@ index fe54eb9..0c47286 100644
+enum ExampleBackedEnum : int
+{
+}
diff --git a/tests/input/Exceptions.php b/tests/input/Exceptions.php
index 9b146c6..3aaa30f 100644
--- a/tests/input/Exceptions.php
+++ b/tests/input/Exceptions.php
@@ -3,3 +3,11 @@
declare(strict_types=1);

namespace Exceptions;
+
+use Exception;
+use Throwable;
+
+try {
+ throw new Exception();
+} catch (Throwable $throwable) {
+}
diff --git a/tests/input/PropertyDeclaration.php b/tests/input/PropertyDeclaration.php
index 0891e12..acdc445 100644
--- a/tests/input/PropertyDeclaration.php
Expand Down