Skip to content

Commit

Permalink
Disallow trigger_error(), which adds dangerous side-effects to PHP …
Browse files Browse the repository at this point in the history
…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).
  • Loading branch information
Ocramius committed Jan 10, 2020
1 parent a5ab9b2 commit 9a1b286
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 5 deletions.
9 changes: 9 additions & 0 deletions lib/Doctrine/ruleset.xml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@
<element key="show_source" value="highlight_file"/>
<element key="sizeof" value="count"/>
<element key="strchr" value="strstr"/>
<!--
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
-->
<element key="trigger_error" value="null"/>
</property>
</properties>
</rule>
Expand Down
4 changes: 2 additions & 2 deletions tests/expected_report.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
----------------------------------------------------------------------
Expand Down
10 changes: 10 additions & 0 deletions tests/fixed/forbidden-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 ');
Expand All @@ -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'
);
10 changes: 10 additions & 0 deletions tests/input/forbidden-functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -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 ');
Expand All @@ -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'
);
6 changes: 3 additions & 3 deletions tests/php-compatibility.patch
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit 9a1b286

Please sign in to comment.