On 02/03/2014 08:38 AM, Stas Malyshev wrote:
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