From 9a1b286a2588f18c1f91163cd091c9a8901386b8 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Fri, 10 Jan 2020 17:28:53 +0100 Subject: [PATCH] Disallow `trigger_error()`, which adds dangerous side-effects to PHP projects As mentioned in detail in the patch, and experienced first-hand in at least 5 real-world projects so far, plus numerous packages within the doctrine ecosystem itself: > Do not use runtime errors as a way to convey deprecations to users. > Warnings, notices, and errors in general (which aren\'t exceptions) are not usable > in downstream projects, and propagate to global error handlers, causing massive issues > in anything relying on STDOUT, STDERR, aggressive logging, or just expects decent performance > from a dependency. In addition to that, introducing additional runtime effects is a potential > BC break I don't remember when we agreed adding any `@trigger_error()` calls to the codebase, but I remember for sure that I stated (clearly) that `@deprecated` is my preferred way, and even with its own limitations, it still is much simpler to deal with downstream (and from a maintenance PoV). --- lib/Doctrine/ruleset.xml | 9 +++++++++ tests/expected_report.txt | 4 ++-- tests/fixed/forbidden-functions.php | 10 ++++++++++ tests/input/forbidden-functions.php | 10 ++++++++++ tests/php-compatibility.patch | 6 +++--- 5 files changed, 34 insertions(+), 5 deletions(-) diff --git a/lib/Doctrine/ruleset.xml b/lib/Doctrine/ruleset.xml index a94a886a..77c47500 100644 --- a/lib/Doctrine/ruleset.xml +++ b/lib/Doctrine/ruleset.xml @@ -78,6 +78,15 @@ + + diff --git a/tests/expected_report.txt b/tests/expected_report.txt index 11b8d1c5..3bcc0a7e 100644 --- a/tests/expected_report.txt +++ b/tests/expected_report.txt @@ -15,7 +15,7 @@ tests/input/duplicate-assignment-variable.php 1 0 tests/input/EarlyReturn.php 6 0 tests/input/example-class.php 34 0 tests/input/forbidden-comments.php 8 0 -tests/input/forbidden-functions.php 6 0 +tests/input/forbidden-functions.php 7 0 tests/input/inline_type_hint_assertions.php 7 0 tests/input/LowCaseTypes.php 2 0 tests/input/namespaces-spacing.php 7 0 @@ -39,7 +39,7 @@ tests/input/use-ordering.php 1 0 tests/input/useless-semicolon.php 2 0 tests/input/UselessConditions.php 20 0 ---------------------------------------------------------------------- -A TOTAL OF 292 ERRORS AND 0 WARNINGS WERE FOUND IN 35 FILES +A TOTAL OF 293 ERRORS AND 0 WARNINGS WERE FOUND IN 35 FILES ---------------------------------------------------------------------- PHPCBF CAN FIX 231 OF THESE SNIFF VIOLATIONS AUTOMATICALLY ---------------------------------------------------------------------- diff --git a/tests/fixed/forbidden-functions.php b/tests/fixed/forbidden-functions.php index f951ca36..0b5673ed 100644 --- a/tests/fixed/forbidden-functions.php +++ b/tests/fixed/forbidden-functions.php @@ -10,6 +10,7 @@ use function is_null; use function settype; use function sizeof; +use function trigger_error; use function var_dump; echo chop('abc '); @@ -28,3 +29,12 @@ extract($bar); compact('foo', 'bar'); + +@trigger_error( + 'Do not use runtime errors as a way to convey deprecations to users. ' + . 'Warnings, notices, and errors in general (which aren\'t exceptions) are not usable ' + . 'in downstream projects, and propagate to global error handlers, causing massive issues ' + . 'in anything relying on STDOUT, STDERR, aggressive logging, or just expects decent performance ' + . 'from a dependency. In addition to that, introducing additional runtime effects is a potential ' + . 'BC break' +); diff --git a/tests/input/forbidden-functions.php b/tests/input/forbidden-functions.php index f951ca36..0b5673ed 100644 --- a/tests/input/forbidden-functions.php +++ b/tests/input/forbidden-functions.php @@ -10,6 +10,7 @@ use function is_null; use function settype; use function sizeof; +use function trigger_error; use function var_dump; echo chop('abc '); @@ -28,3 +29,12 @@ extract($bar); compact('foo', 'bar'); + +@trigger_error( + 'Do not use runtime errors as a way to convey deprecations to users. ' + . 'Warnings, notices, and errors in general (which aren\'t exceptions) are not usable ' + . 'in downstream projects, and propagate to global error handlers, causing massive issues ' + . 'in anything relying on STDOUT, STDERR, aggressive logging, or just expects decent performance ' + . 'from a dependency. In addition to that, introducing additional runtime effects is a potential ' + . 'BC break' +); diff --git a/tests/php-compatibility.patch b/tests/php-compatibility.patch index 273d4c32..ca48b8be 100644 --- a/tests/php-compatibility.patch +++ b/tests/php-compatibility.patch @@ -9,7 +9,7 @@ index 11b8d1c..5072a64 100644 -tests/input/example-class.php 34 0 +tests/input/example-class.php 37 0 tests/input/forbidden-comments.php 8 0 - tests/input/forbidden-functions.php 6 0 + tests/input/forbidden-functions.php 7 0 tests/input/inline_type_hint_assertions.php 7 0 @@ -33,15 +33,15 @@ tests/input/superfluous-naming.php 11 0 tests/input/test-case.php 8 0 @@ -22,8 +22,8 @@ index 11b8d1c..5072a64 100644 tests/input/useless-semicolon.php 2 0 tests/input/UselessConditions.php 20 0 ---------------------------------------------------------------------- --A TOTAL OF 292 ERRORS AND 0 WARNINGS WERE FOUND IN 35 FILES -+A TOTAL OF 296 ERRORS AND 0 WARNINGS WERE FOUND IN 35 FILES +-A TOTAL OF 293 ERRORS AND 0 WARNINGS WERE FOUND IN 35 FILES ++A TOTAL OF 297 ERRORS AND 0 WARNINGS WERE FOUND IN 35 FILES ---------------------------------------------------------------------- -PHPCBF CAN FIX 231 OF THESE SNIFF VIOLATIONS AUTOMATICALLY +PHPCBF CAN FIX 235 OF THESE SNIFF VIOLATIONS AUTOMATICALLY