Morning Internals [, and Dmitry :)],
I came across a reason to think about assertions again today, my
original, pretty radical, patch was worked on by dmitry, I updated the
RFC a while ago but allowed the conversation to die down.
Nikita pointed this out around the same time:
https://gist.github.com/krakjoe/4a2145e5a6ddc26b1dc1
Still, there's no reason for us to have a crappy assertion api as part
of the core just because there's a way around it(-ish) in user land.
So, I think now is a good time to revive the discussion, the patch is
pretty unobjectionable, and has been updated for master.
I'd like to move this to a vote pretty swiftly, any objections ?
Cheers
Joe
Morning Internals [, and Dmitry :)],
I came across a reason to think about assertions again today, myoriginal, pretty radical, patch was worked on by dmitry, I updated the
RFC a while ago but allowed the conversation to die down.Nikita pointed this out around the same time: https://gist.github.com/krakjoe/4a2145e5a6ddc26b1dc1 Still, there's no reason for us to have a crappy assertion api aspart of the core just because there's a way around it(-ish) in user land.
So, I think now is a good time to revive the discussion, the patchis pretty unobjectionable, and has been updated for master.
I'd like to move this to a vote pretty swiftly, any objections ?Cheers
Joe
Woops2,
https://gist.github.com/krakjoe/c6bcbb4e6d019c9297db
Dunno what happened there ..
Cheers
Joe
Hi!
Still, there's no reason for us to have a crappy assertion api as part
of the core just because there's a way around it(-ish) in user land.
I think there is some reason - if it can easily be done in userland,
with more flexibility and suitability for particular purposes of the
user, why put it in core? If it's such a common feature, make a PSR for
it and create reference implementation that everybody would be using. So
my question is - what having in core adds to it that can't be done as
easily in userland?
Also, it essentially splits assert() into two completely independent
things one of them being regular function that you can call and another
being weird magic thing. Moreover, for the same assert there's now two
completely independent ways of controlling it - one is zend.assert and
another is assert_options/ASSERT_ACTIVE. E.g. compare:
// This is controlled by zend.assert
assert($foo > 1, "Foo is too small");
// This is controlled by assert_options
$f = 'assert';
$f($foo > 1, "Foo is too small");
This looks a bit strange for me.
Also, assert() will throw exception if assert.bail = 1 and
assert.exception = 1, but only if the argument is not a string. If the
argument is a string no exception will be thrown. OTOH, if the argument
IS a string, assert will both throw exception and bail out, which is
not clear what is the purpose of that.
I'm also feeling a bit uneasy with adding more and more special cases
for special names in the engine. Maybe we could create some more generic
facility that would not need to be one-off case for just one function?
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
Still, there's no reason for us to have a crappy assertion api as part
of the core just because there's a way around it(-ish) in user land.
I think there is some reason - if it can easily be done in userland,
with more flexibility and suitability for particular purposes of the
user, why put it in core? If it's such a common feature, make a PSR for
it and create reference implementation that everybody would be using. So
my question is - what having in core adds to it that can't be done as
easily in userland?Also, it essentially splits
assert()into two completely independent
things one of them being regular function that you can call and another
being weird magic thing. Moreover, for the same assert there's now two
completely independent ways of controlling it - one is zend.assert and
another is assert_options/ASSERT_ACTIVE. E.g. compare:// This is controlled by zend.assert
assert($foo > 1, "Foo is too small");// This is controlled by assert_options
$f = 'assert';
$f($foo > 1, "Foo is too small");This looks a bit strange for me.
Also,
assert()will throw exception if assert.bail = 1 and
assert.exception = 1, but only if the argument is not a string. If the
argument is a string no exception will be thrown. OTOH, if the argument
IS a string, assert will both throw exception and bail out, which is
not clear what is the purpose of that.I'm also feeling a bit uneasy with adding more and more special cases
for special names in the engine. Maybe we could create some more generic
facility that would not need to be one-off case for just one function?
Well lets not forget that we already have an implementation as part of
the core, so to argue that we don't require assert in the core is moot;
we already have it.
While you can avoid some overhead in userland, you cannot remove the
overhead completely; a good assertion API should have no impact on
production, none, it should require no boilerplate code to make sane use
of it, at all, since it is meant to be a core feature.
It would seem to create inconsistency, but only because it's compatible
with legacy code asserting strings, if you are smart, you will not
assert with strings and take advantage of the new implementation, if you
are not, you will continue to assert strings and be unaffected by the
new implementation ... that seems ideal to me, and Dmitry ...
Cheers
Joe
Hi!
Well lets not forget that we already have an implementation as part of
the core, so to argue that we don't require assert in the core is moot;
we already have it.
I'm not sure what you mean. We have assert function, a function among
others. What you propose is special engine-level magic just for this
function. The difference is like having a program that runs on Linux and
does X and having special patch in Linux kernel just to do X. The bar
for the latter is much higher.
While you can avoid some overhead in userland, you cannot remove the
overhead completely; a good assertion API should have no impact on
production, none, it should require no boilerplate code to make sane use
of it, at all, since it is meant to be a core feature.
This is not completely fulfilled by your patch either - it still has an
overhead of having the opcodes there and doing the jumps. If the code
you're avoiding to run is heavy, then of course this overhead is
negligible compared to the code you're avoiding, but the same then is
true for purely userspace solution.
It would seem to create inconsistency, but only because it's compatible
with legacy code asserting strings, if you are smart, you will not
It doesn't "seem" to create inconsistency. It creates it. It's not like
I'm imagining two parameters doing the same thing :)
assert with strings and take advantage of the new implementation, if you
are not, you will continue to assert strings and be unaffected by the
new implementation ... that seems ideal to me, and Dmitry ...
I don't think having a function that is half a function and half a magic
engine constraint and that is controlled by two separate settings, one
living in zend. and one in assert. is really ideal. Also, it is not true
that string parameter is the only thing that controls it - as I already
showed, how you call it also makes it work completely different and
assert.* parameters still influence it even in magic mode, leading to
such things as both throwing exceptions and bailing out - which I still
don't understand how it's supposed to work.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
Well lets not forget that we already have an implementation as part of
the core, so to argue that we don't require assert in the core is moot;
we already have it.
I'm not sure what you mean. We have assert function, a function among
others. What you propose is special engine-level magic just for this
function. The difference is like having a program that runs on Linux and
does X and having special patch in Linux kernel just to do X. The bar
for the latter is much higher.While you can avoid some overhead in userland, you cannot remove the
overhead completely; a good assertion API should have no impact on
production, none, it should require no boilerplate code to make sane use
of it, at all, since it is meant to be a core feature.
This is not completely fulfilled by your patch either - it still has an
overhead of having the opcodes there and doing the jumps. If the code
you're avoiding to run is heavy, then of course this overhead is
negligible compared to the code you're avoiding, but the same then is
true for purely userspace solution.It would seem to create inconsistency, but only because it's compatible
with legacy code asserting strings, if you are smart, you will not
It doesn't "seem" to create inconsistency. It creates it. It's not like
I'm imagining two parameters doing the same thing :)assert with strings and take advantage of the new implementation, if you
are not, you will continue to assert strings and be unaffected by the
new implementation ... that seems ideal to me, and Dmitry ...
I don't think having a function that is half a function and half a magic
engine constraint and that is controlled by two separate settings, one
living in zend. and one in assert. is really ideal. Also, it is not true
that string parameter is the only thing that controls it - as I already
showed, how you call it also makes it work completely different and
assert.* parameters still influence it even in magic mode, leading to
such things as both throwing exceptions and bailing out - which I still
don't understand how it's supposed to work.
This is not completely fulfilled by your patch either - it still has an
overhead of having the opcodes there and doing the jumps. If the code
you're avoiding to run is heavy, then of course this overhead is
negligible compared to the code you're avoiding, but the same then is
true for purely userspace solution.
As it says in the RFC, the complete removal will be implemented before
merge, the implementation allows the complete removal, the one in the
core doesn't.
The original patch didn't have this limitation but wasn't compatible
with the existing assert API, so dmitry decided this way was better.
It doesn't "seem" to create inconsistency. It creates it. It's not like
I'm imagining two parameters doing the same thing :)
It doesn't create inconsistency in code currently using assert, and
there's no such thing as inconsistent code that doesn't exist, so I
don't see the inconsistency you think is definitely there, I see how you
think it's there, without really considering it, but it is not in fact
there. Code currently using assert will behave in the same way, code
taking advantage of exceptions and expressions as arguments will behave
in the way the patch dictates. I see no inconsistency.
I don't think having a function that is half a function and half a magic
engine constraint and that is controlled by two separate settings, one
living in zend. and one in assert. is really ideal. Also, it is not true
that string parameter is the only thing that controls it - as I already
showed, how you call it also makes it work completely different and
assert.* parameters still influence it even in magic mode, leading to
such things as both throwing exceptions and bailing out - which I still
don't understand how it's supposed to work.
Nor do I, the original patch wasn't this, it was a new syntax with this
API that solved the problems this patch is solving in the same way but
by superseding it's functionality rather than trying to retain
compatibility with something that is crappy. Since that turned out to
not be an option - we have to retain compatibility with the current
assert api - the current patch is the only way of moving forward, even
if you can see ways of making it do strange things (as a result of
retaining compatibility), the perfectly clean patch, with no strangeness
whatsoever, was rejected because we don't like to introduce new keywords
or syntax or blah blah blah ...
If you use the configuration you had before it will behave as it did
before, if you setup the new configuration and use the new api it will
behave as the patch describes, that you can purposefully configure or
otherwise force php to do strange things is nothing new.
I'm going to move it to vote today, there's not much point in talking
about it, the discussions have already been had and the patch rewritten
to satisfy the complaints of most people, and has also been available
for many many months, so if there is a suitable patch, this is it, I
don't want to waste more time on it if we're not going to move forward.
Cheers
Joe
Hi!
Well lets not forget that we already have an implementation as part of
the core, so to argue that we don't require assert in the core is moot;
we already have it.I'm not sure what you mean. We have assert function, a function among
others. What you propose is special engine-level magic just for this
function. The difference is like having a program that runs on Linux and
does X and having special patch in Linux kernel just to do X. The bar
for the latter is much higher.While you can avoid some overhead in userland, you cannot remove the
overhead completely; a good assertion API should have no impact on
production, none, it should require no boilerplate code to make sane use
of it, at all, since it is meant to be a core feature.This is not completely fulfilled by your patch either - it still has an
overhead of having the opcodes there and doing the jumps. If the code
you're avoiding to run is heavy, then of course this overhead is
negligible compared to the code you're avoiding, but the same then is
true for purely userspace solution.It would seem to create inconsistency, but only because it's compatible
with legacy code asserting strings, if you are smart, you will notIt doesn't "seem" to create inconsistency. It creates it. It's not like
I'm imagining two parameters doing the same thing :)assert with strings and take advantage of the new implementation, if you
are not, you will continue to assert strings and be unaffected by the
new implementation ... that seems ideal to me, and Dmitry ...I don't think having a function that is half a function and half a magic
engine constraint and that is controlled by two separate settings, one
living in zend. and one in assert. is really ideal. Also, it is not true
that string parameter is the only thing that controls it - as I already
showed, how you call it also makes it work completely different and
assert.* parameters still influence it even in magic mode, leading to
such things as both throwing exceptions and bailing out - which I still
don't understand how it's supposed to work.
I fully agree here.
As the effort to reduce the impact of assert in production is much
appreciated I do not see much needs for this "addition".
I also discussed with a couple of developers being intensively
involved in testing tools and framework for php and the consensus is
that they do not need it for the large majority. A minority even
considers this feature as not really good, assert never was a good
thing to use (I never used for example).
Cheers,
Pierre
@pierrejoye | http://www.libgd.org
Hi Pierre,
I also discussed with a couple of developers being intensively
involved in testing tools and framework for php and the consensus is
that they do not need it for the large majority. A minority even
considers this feature as not really good, assert never was a good
thing to use (I never used for example).
For testers and test tools developers, or even framework developer,
assertion
may not be important. Framework may have contract via annotations.
However, it is important for developers who is not rely on framework to
make
sure data is processed as developer expects.
It would be important for portable library developers especially. Since
they do not
want to rely on specific frameworks nor they cannot assume user tests/code
are
valid one. Assertion works perfectly for portable library developers to
achieve
both performance and reliability.
Assertion is useful for basic form of DbC. DbC could achieve fast, yet
reliable
applications. Extensive use of assertion is required if developer choose to
do
so. It is important to remove assertion overheads from production code.
Therefore, this RFC is valuable people who care about DbC and prefer to use
light weight frameworks like myself.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Yasuo Ohgaki wrote:
It is important to remove assertion overheads from production code.
Therefore, this RFC is valuable people who care about DbC and prefer to use
light weight frameworks like myself.
And when did we agree to support DbC in the core?
It really is feeling as if there are two versions of PHP being developed ... the
one everybody uses and another 'fancy version' with all sorts of bells and
whistles and just a very small minority want.
Why can't all of these extensions simply be supported by third party tools ...
which are already available ... and keep the core as simple as possible.
--
Lester Caine - G8HFL
Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk
Hi Lester,
Yasuo Ohgaki wrote:
It is important to remove assertion overheads from production code.
Therefore, this RFC is valuable people who care about DbC and prefer to
use
light weight frameworks like myself.And when did we agree to support DbC in the core?
It really is feeling as if there are two versions of PHP being developed
... the one everybody uses and another 'fancy version' with all sorts of
bells and whistles and just a very small minority want.
Why can't all of these extensions simply be supported by third party tools
... which are already available ... and keep the core as simple as possible.
assert() is not new feature, but this RFC is for improving it.
Current assert() will call NULL function at run time. Therefore, there
would be overheads that cannot be ignored when assert() is used
extensively. With this RFC, run time overheads are removed.
I agree that this RFC would change the way script is coded.
Assertion can be used for DbC, but it's up to users if they use it for DbC
or not.
No existing code is affected in any way.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
I support the idea behind improving assert; I really should be able to say
'disable assertions' somehow and then assertions would have no impact at
runtime. I have not reviewed the changes for the revised RFC at this time,
but I wanted to voice my support for the general idea.
Also:
assert never was a good thing to use
This is just nonsense. Properly used asserts are fundamental in defensive
programming and you can quite a bit of literature about it. Perhaps Pierre
meant to say, "assert never was a good thing to use in PHP" and then I
would agree with him because of the currently poor implementation, not
because they are bad in principle.
I support the idea behind improving assert; I really should be able to
say 'disable assertions' somehow and then assertions would have no impact
at runtime. I have not reviewed the changes for the revised RFC at this
time, but I wanted to voice my support for the general idea.Also:
assert never was a good thing to use
This is just nonsense. Properly used asserts are fundamental in defensive
programming and you can quite a bit of literature about it. Perhaps Pierre
meant to say, "assert never was a good thing to use in PHP"
Indeed in php, and I do not use it in c either btw.
Hi Stas,
In main opinion, the main advantage of the proposal is ability to use of
assert() function in production code with zero cost.
It might be important, because cost of the assert() call and condition
evaluation might be expensive.
It's similar to C which provides assert() function, but near every project
defines ASSERT macro anyway, to eliminate assert() calls for release build.
Thanks. Dmitry.
On Mon, Feb 3, 2014 at 12:38 PM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
Well lets not forget that we already have an implementation as part of
the core, so to argue that we don't require assert in the core is moot;
we already have it.I'm not sure what you mean. We have assert function, a function among
others. What you propose is special engine-level magic just for this
function. The difference is like having a program that runs on Linux and
does X and having special patch in Linux kernel just to do X. The bar
for the latter is much higher.While you can avoid some overhead in userland, you cannot remove the
overhead completely; a good assertion API should have no impact on
production, none, it should require no boilerplate code to make sane use
of it, at all, since it is meant to be a core feature.This is not completely fulfilled by your patch either - it still has an
overhead of having the opcodes there and doing the jumps. If the code
you're avoiding to run is heavy, then of course this overhead is
negligible compared to the code you're avoiding, but the same then is
true for purely userspace solution.It would seem to create inconsistency, but only because it's compatible
with legacy code asserting strings, if you are smart, you will notIt doesn't "seem" to create inconsistency. It creates it. It's not like
I'm imagining two parameters doing the same thing :)assert with strings and take advantage of the new implementation, if you
are not, you will continue to assert strings and be unaffected by the
new implementation ... that seems ideal to me, and Dmitry ...I don't think having a function that is half a function and half a magic
engine constraint and that is controlled by two separate settings, one
living in zend. and one in assert. is really ideal. Also, it is not true
that string parameter is the only thing that controls it - as I already
showed, how you call it also makes it work completely different and
assert.* parameters still influence it even in magic mode, leading to
such things as both throwing exceptions and bailing out - which I still
don't understand how it's supposed to work.--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
Hi!
In main opinion, the main advantage of the proposal is ability to use of
assert()function in production code with zero cost.
It might be important, because cost of theassert()call and condition
evaluation might be expensive.
I agree, this would be nice. Though it can be implemented using closures
and calling function only when needed. Granted, it is less convenient
and I'd like to have generic conditional compilation facility that could
help with it (or compiler/optimizer smart enough that if(<constant
false>) could be eliminated by it) but doing it as one-off hack for one
specific function doesn't look like ideal solution to me.
BTW, current patch has zend.assertions as INI_SYSTEM, which means the
whole server can either have assertions on or off. Which means if you
want to have two sites with different settings (like staging and
production, or production and debug copy), you'd have to set up separate
PHP server for each.
It's similar to C which provides
assert()function, but near every
project defines ASSERT macro anyway, to eliminateassert()calls for
release build.
In C, you can do it with NDEBUG setting, which works a bit differently -
you compile it once, and then it's predictable forever. With PHP it's
not that easy, unfortunately.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
On Tue, Feb 4, 2014 at 1:58 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:
Hi!
In main opinion, the main advantage of the proposal is ability to use of
assert()function in production code with zero cost.
It might be important, because cost of theassert()call and condition
evaluation might be expensive.I agree, this would be nice. Though it can be implemented using closures
and calling function only when needed. Granted, it is less convenient
and I'd like to have generic conditional compilation facility that could
help with it (or compiler/optimizer smart enough that if(<constant false>) could be eliminated by it)
Unfortunately, optimizer is not smart enough to solve this problem (on
purpose).
It may substitute only internal constants and constants defined in the same
file
but doing it as one-off hack for one
specific function doesn't look like ideal solution to me.
I wouldn't say it's an ideal solution. It's more a compromise that allows
safer checks in debugging and running in production without performance
lose. The solution also keeps full BC with old assert() behavior, that
wasn't trivial :)
BTW, current patch has zend.assertions as INI_SYSTEM, which means the
whole server can either have assertions on or off.
It's INI_SYSTEM on purpose. zend.assertions is a 3 way switch that may
disable code generation for assert().
Changing it after PHP files are already cached won't make any effect.
Which means if you
want to have two sites with different settings (like staging and
production, or production and debug copy), you'd have to set up separate
PHP server for each.
yes.
It's similar to C which provides
assert()function, but near every
project defines ASSERT macro anyway, to eliminateassert()calls for
release build.In C, you can do it with NDEBUG setting, which works a bit differently -
you compile it once, and then it's predictable forever. With PHP it's
not that easy, unfortunately.
So, C provides special handling for assert() as well :)
Thanks. Dmitry.
--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227
BTW, current patch has zend.assertions as INI_SYSTEM, which means the
whole server can either have assertions on or off.
It'sINI_SYSTEMon purpose. zend.assertions is a 3 way switch that may
disable code generation forassert().
Changing it after PHP files are already cached won't make any effect.
Incidentally, if this is true, the introduction in the RFC is incorrect,
as it currently states:
assertions can therefore be disabled and enabled via the PHP_INI_ALL
configuration setting zend.assertions
The explanation of why it would be INI_SYSTEM could also be added there.
--
Rowan Collins
[IMSoP]
It's INI_SYSTEM because it may affect the compiled code that might be
cached.
So changing it on second request won't affect already cache code.
Thanks. Dmitry.
On Wed, Feb 5, 2014 at 12:57 AM, Rowan Collins rowan.collins@gmail.comwrote:
BTW, current patch has zend.assertions as INI_SYSTEM, which means the
whole server can either have assertions on or off.
It's
INI_SYSTEMon purpose. zend.assertions is a 3 way switch that may
disable code generation forassert().
Changing it after PHP files are already cached won't make any effect.Incidentally, if this is true, the introduction in the RFC is incorrect,
as it currently states:assertions can therefore be disabled and enabled via the PHP_INI_ALL
configuration setting zend.assertionsThe explanation of why it would be
INI_SYSTEMcould also be added there.--
Rowan Collins
[IMSoP]
While you can avoid some overhead in userland, you cannot remove the
overhead completely; a good assertion API should have no impact on
production, none, it should require no boilerplate code to make sane
use of it, at all, since it is meant to be a core feature.
I think Stas' main objection was the inconsistencies that the "new
assert" method introduces. And I have to agree with that there. It
shouldn't depend on whether you use a string or not how the
construct/function changes code execution and exception throwing.
cheers,
Derick
--
http://derickrethans.nl | http://xdebug.org
Like Xdebug? Consider a donation: http://xdebug.org/donate.php
twitter: @derickr and @xdebug
Posted with an email client that doesn't mangle email: alpine
Morning Internals [, and Dmitry :)],
I came across a reason to think about assertions again today, myoriginal, pretty radical, patch was worked on by dmitry, I updated the
RFC a while ago but allowed the conversation to die down.Nikita pointed this out around the same time: https://gist.github.com/krakjoe/4a2145e5a6ddc26b1dc1 Still, there's no reason for us to have a crappy assertion api aspart of the core just because there's a way around it(-ish) in user land.
So, I think now is a good time to revive the discussion, the patchis pretty unobjectionable, and has been updated for master.
I'd like to move this to a vote pretty swiftly, any objections ?Cheers
Joe
This is just a comment from someone without massively strong opinions on
the proposal, but to me the RFC doesn't really make clear which parts of
the proposed version of assert() are actually new. For instance, all the
examples in the "Scope of Assertions" section are actually valid in
current PHP, and if written to use eval()d strings could be used as the
manual page for assert() today.
Without diving into the patch, I understand there to be two, somewhat
independent, changes proposed here:
-
An ability to by-pass the asserted expression in the compiler but lay
it out as plain PHP code, rather than an eval()'d string, combining the
advantages of both versions currently supported. Some of your posts
suggest that it is also more efficient even than the eval()d string
version, but this isn't really clarified in the RFC. A new ini setting,
zend.assert, is added to replace (or supplement?) assert.active to
control this part of the behaviour. -
A mode to throw Exceptions, rather than Warnings or just bailing,
when an assertion fails. The first part of this is an extra assert
option, assert.exceptions, which will cause the assertion to throw an
AssertionException if it fails.
In addition, the second parameter ($message) is extended so that if an
object descending from AssertException is passed, that will be thrown
instead. Passing such an object in is probably only efficient in
conjunction with the ability of the compiler to skip the entire
expression, so somewhat relies on both of the above features.
Does this sound like a fair summary? If so, perhaps it could be added as
an introduction in the RFC?
Regards,
--
Rowan Collins
[IMSoP]