My take on assertions, which I also tried to stick to in Wikibase, is as follows:
* A failing assertion indicates a "local" error in the code or a bug in PHP; They should not be used to check preconditions or validate input. That's what InvalidArgumentException is for (and I wish type hints would trigger that, and not a "fatal error"). Precondition checks can always fail, never trust the caller. Assertions are things that should *always* be true.
* Use assertions to check postconditions (and perhaps invariants). That is, use them to assert that the code in the method (and maybe class) that contains the assert is correct. Do not use them to enforce caller behavior.
* Use boolean expressions in assertions, not strings. The speed advantage of strings is not big, since the expression should be a very basic one anyway, and strings are awkward to read, write, and, as mentioned before, potentially dangerous, because they are eval()ed.
* The notion of "bailing out" on "fatal errors" is a misguided remnant from the days when PHP didn't have exceptions. In my mind, assertions should just throw an (usually unhandled) exception, like Java's AssertionError.
I think if we stick with this, assertions are potentially useful, and harmless at worst. But if there is consensus that they should not be used anywhere, ever, we'll remove them. I don't really see how the resulting boiler plate would be cleaner or safer:
if ( $foo > $bar ) { throw new OMGWTFError(); }
-- daniel
Am 31.07.2013 00:28, schrieb Tim Starling:
On 31/07/13 07:28, Max Semenik wrote:
I remeber we discussed using asserts and decided they're a bad idea for WMF-deployed code - yet I see
Warning: assert() [<a href='function.assert'>function.assert</a>]: Assertion failed in /usr/local/apache/common-local/php-1.22wmf12/extensions/WikibaseDataModel/DataModel/Claim/Claims.php on line 291
The original discussion is here:
http://thread.gmane.org/gmane.science.linguistics.wikipedia.technical/59620
Judge for yourself.
-- Tim Starling
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Hi,
On Wed, Jul 31, 2013 at 10:36:56AM +0200, Daniel Kinzler wrote:
- Use boolean expressions in assertions, not strings.
I do not agree that this is best practice in PHP.
Execution time being only part of argument here. Among other arguments are readability of the error message. When using strings, the error message contains the condition of the failed assertion.
But as has been pointed out by other posts in this thread, correct quotation of the string is obviously crucial.
The speed advantage of strings is not big, since the expression should be a very basic one anyway, [...]
They should? In practice they typically are not. For example
assert( $this->indicesAreUpToDate() );
of WikibaseDataModel/DataModel/Claim/Claims.php boils down to nested looping, if I read the code correctly. But (leaving the question aside whether that part is misusing assertions) using complex predicates is totally ok (and even useful) when putting them in strings, as the expression would not get evaluated in production anyways.
- The notion of "bailing out" on "fatal errors" is a misguided remnant
from the days when PHP didn't have exceptions. In my mind, assertions should just throw an (usually unhandled) exception
We could get that by adapting assert_options ...
, like Java's AssertionError.
That comparison is ill suited. Java's AssertionError is a java.lang.Error and not java.lang.Exception. And thereby it's clear what the Java world thinks about catching assertion failures [1]:
An Error [...] indicates serious problems that a reasonable application should not try to catch.
But then again… maybe that was what you meant by “usually unhandled” anyways.
I don't really see how the resulting boiler plate would be cleaner or safer:
if ( $foo > $bar ) { throw new OMGWTFError(); }
Totally! It looks even less clean to me, as after such guards only the negated condition holds. So (when not misusing them) asserts are a way to reduce complexity of reading code.
Best regards, Christian
[1] http://docs.oracle.com/javase/7/docs/api/java/lang/Error.html but this intepretation stands since around Java 1.4 and has proven useful.
On 31/07/13 18:36, Daniel Kinzler wrote:
Assertions are things that should *always* be true.
In my mind, assertions should just throw an (usually unhandled) exception, like Java's AssertionError.
Indeed. In C, assert() will abort the program if it is enabled, which is hard to miss. It is not comparable to the PHP assert() function.
I don't really see how the resulting boiler plate would be cleaner or safer:
if ( $foo > $bar ) { throw new OMGWTFError(); }
The reasons I don't like assert() are:
1. It doesn't throw an exception 2. It acts like eval()
We could have a library of PHPUnit-style assertion functions which throw exceptions and don't act like eval(), I would be fine with that. Maybe MWAssert::greaterThan( $foo, $bar ) or something.
-- Tim Starling
On Wed, Jul 31, 2013 at 7:42 AM, Tim Starling tstarling@wikimedia.orgwrote:
Indeed. In C, assert() will abort the program if it is enabled, which is hard to miss. It is not comparable to the PHP assert() function.
...except PHP's assert() *also* aborts the program if enabled. What am I missing here?
The reasons I don't like assert() are:
- It doesn't throw an exception
- It acts like eval()
We could have a library of PHPUnit-style assertion functions which throw exceptions and don't act like eval(), I would be fine with that. Maybe MWAssert::greaterThan( $foo, $bar ) or something.
1. It's fairly trivial to use assert_options() to make assertions throw exceptions if you really wanted to while developing. 2. Except it's not. Again, you're welcome to give an example where code provided as a string in an assertion is not exactly the same as having the code hardcoded.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
$_GET["foo"] = 'include( "evil_file.php" )'; assert( '$_GET["foo"] == "fluffy bunny rabbit"' ); // This is fine assert( "$_GET['foo'] == 'fluffy bunny rabbit'" ); // But this is not
Deliberately using a function which reduces the security of your application to relying on everyone choosing the correct type of quotes is definitely asking for trouble.
--HM
On 31 July 2013 13:19, Tyler Romeo tylerromeo@gmail.com wrote:
On Wed, Jul 31, 2013 at 7:42 AM, Tim Starling <tstarling@wikimedia.org
wrote:
Indeed. In C, assert() will abort the program if it is enabled, which is hard to miss. It is not comparable to the PHP assert() function.
...except PHP's assert() *also* aborts the program if enabled. What am I missing here?
The reasons I don't like assert() are:
- It doesn't throw an exception
- It acts like eval()
We could have a library of PHPUnit-style assertion functions which throw exceptions and don't act like eval(), I would be fine with that. Maybe MWAssert::greaterThan( $foo, $bar ) or something.
- It's fairly trivial to use assert_options() to make assertions throw
exceptions if you really wanted to while developing. 2. Except it's not. Again, you're welcome to give an example where code provided as a string in an assertion is not exactly the same as having the code hardcoded.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2016 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com _______________________________________________ Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
Am 31.07.2013 13:42, schrieb Tim Starling:
We could have a library of PHPUnit-style assertion functions which throw exceptions and don't act like eval(), I would be fine with that. Maybe MWAssert::greaterThan( $foo, $bar ) or something.
I like that! Should support an error message as an optional parameter. I suppose we could just steal the method signatures from phpunit.
-- daniel
I like the idea of an MWAssert and that was on my personal TODO list for some time (down to the class name). It would indeed allow for unobtrusive one line "unchecked" exceptions.
On Wed, Jul 31, 2013 at 4:42 AM, Tim Starling tstarling@wikimedia.orgwrote:
On 31/07/13 18:36, Daniel Kinzler wrote:
Assertions are things that should *always* be true.
In my mind, assertions should just throw an (usually unhandled) exception, like Java's AssertionError.
Indeed. In C, assert() will abort the program if it is enabled, which is hard to miss. It is not comparable to the PHP assert() function.
I don't really see how the resulting boiler plate would be cleaner or safer:
if ( $foo > $bar ) { throw new OMGWTFError(); }
The reasons I don't like assert() are:
- It doesn't throw an exception
- It acts like eval()
We could have a library of PHPUnit-style assertion functions which throw exceptions and don't act like eval(), I would be fine with that. Maybe MWAssert::greaterThan( $foo, $bar ) or something.
-- Tim Starling
Wikidata-tech mailing list Wikidata-tech@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikidata-tech
wikidata-tech@lists.wikimedia.org