Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 02/03/15 09:47, Zeev Suraski wrote: I'm obviously biased but I believe that the coercive rules actually cover a lot more ground than the weak+strict sets. In other words, with weak+strict, overall, you're going to have to add custom code in a lot more cases than with the coercive rule set (either because weak is too weak and strict will very frequently be too strict, while coercive provides a rule-set that accepts sensible values and reject non-sensible ones, making it work out of the box in most cases). But even with coercive, it's definitely not a one size fits all solution, and it's not supposed to be either. You still have custom code available to you. But since the horse is now out of the stable, we have to live with strict anyway, so where will coercive rules fit in if they get accepted as well? All of this is just creating different rules for sub-sets of users :( -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
-Original Message- From: Lester Caine [mailto:les...@lsces.co.uk] Sent: Monday, March 02, 2015 12:10 PM To: Zeev Suraski Cc: internals@lists.php.net Subject: Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC On 02/03/15 09:47, Zeev Suraski wrote: I'm obviously biased but I believe that the coercive rules actually cover a lot more ground than the weak+strict sets. In other words, with weak+strict, overall, you're going to have to add custom code in a lot weak+more cases than with the coercive rule set (either because weak is too weak and strict will very frequently be too strict, while coercive provides a rule-set that accepts sensible values and reject non-sensible ones, making it work out of the box in most cases). But even with coercive, it's definitely not a one size fits all solution, and it's not supposed to be either. You still have custom code available to you. But since the horse is now out of the stable, we have to live with strict anyway, so where will coercive rules fit in if they get accepted as well? All of this is just creating different rules for sub-sets of users :( As Anthony proposed if both pass, we're going to have a vote to decide between them, as they do contradict each other. Note that we're not going to know the status of either vote with certainty until March 13th. Zeev -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
-Original Message- From: Thomas Punt [mailto:tp...@hotmail.co.uk] Sent: Sunday, March 01, 2015 4:03 PM To: Zeev Suraski; PHP internals Subject: RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC Hey Zeev, Another change being considered and not yet in the RFC is re-allowing leading and trailing spaces for numeric strings (sorry Paddy.) I think that rejecting leading and trailing spaces for stringy ints is for the best. If I only want to accept an integer (in either int or string form), then by enabling spaces in that input, it would destroy the overall usability of it. One scenario I have in mind for this is validating $_GET information for a RESTful web service. Having potentially an infinite number of URIs that all point to the same resource isn't good: /users/1 /users/%201 /users/1%20 /users/%201%20 /users/%201%20%20 etc. In this case, I don't want to accept any leading or trailing spaces for the user ID. So I would therefore not be able to use an `int` type hint because its acceptance rules would be too lax. If spaces are not accepted in stringy ints, and I want to pass a stringy int that may have leading or trailing spaces in it, then I know I can simply apply a trim() to it before passing it into a function that's expecting only an int. This way, the usability of integer-only inputs (as string or ints) isn't compromised by the rules being too weak. Tom, First of all thanks for the feedback! I think that with leading/trailing spaces we could go either way. It boils down to the question of whether we want spaces to be explicitly trim()'d and have the rule-set more restrictive, or have the rule-set more permissive - preventing some use cases from using it and having to do manual validation instead. Based on the fact this has been a source of back forth changes (twitter.com/AndreaFaulds/status/570979956349665281), there's not going to be a one size fits all rule-set here. I think that the use cases where it will be harmless to ignore leading/trailing spaces would be more common than those where it's risky or undesired... The takeaway from this seems to be that the approach of tightening the existing rule-set and applying it to both internal functions and new user-land code is viable, and does not cause the sky to come down falling. Preliminary tests suggest it actually finds real potential problems. More tomorrow. Overall, a big +1 on this though! The type coercion rules at the moment are far too lax to be considered useful in most situations IMO. Thanks for the feedback! Zeev -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
-Original Message- From: Lester Caine [mailto:les...@lsces.co.uk] Sent: Monday, March 02, 2015 11:31 AM To: internals@lists.php.net Subject: Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC Andrea's post highlights the fact that we did try a fix when PHP5 came out. What it fails to add is perhaps why the change was reverted? Still today there are people pressuring to have it restored? As this thread has shown. At the end of the day, this fine tuning of casting action has very little to do with the type hinting debate? Now IS the time to discuss the rules but not as part of some other RFC? It deserves it's own independent discussion as it SHOULD apply what ever happens over type hinting. The whole discussion is presently limited to the area of type hints, and this is unlikely to change for 7.0. I think it's good to think about post 7.0 changes - perhaps bringing the rule-set to other areas of PHP (namely implicit casting), but no, it's not the time to seriously discuss this IMHO. Thomas's example in my book is where a number of things come into play? My first thought would be just where is this actually processed and so where is it trimmed? Additionally how about '001'? However it does highlight that a single 'int' hint is not going to satisfy everybody anyway? The thing perhaps to point out here is that we are looking in this case at a source that may well be using unicode and wonder if THAT may not be more of a problem here? Whether we have weak, strict, coercive or yellow-red type hints added to PHP, they're absolutely not supposed to satisfy everyone. They're supposed to satisfy most use cases, but not all - for the remaining X percent, you'd have to write a bit of custom code, which is fine (either do away with the hint and do custom validation, or use the hint - but do a bit of preprocessing in the calling code). It's not as if everything we deal with in scalars is going to fit into the various scalar type hint buckets anyway. Functions that accept URLs, or emails, or other types of scalar-but-specifically-formatted data have to be validated with custom code anyway. It's fine for other cases to require a bit of custom validation - as long as the key use cases are covered. I'm obviously biased but I believe that the coercive rules actually cover a lot more ground than the weak+strict sets. In other words, with weak+strict, overall, you're going to have to add custom code in a lot more cases than with the coercive rule set (either because weak is too weak and strict will very frequently be too strict, while coercive provides a rule-set that accepts sensible values and reject non-sensible ones, making it work out of the box in most cases). But even with coercive, it's definitely not a one size fits all solution, and it's not supposed to be either. You still have custom code available to you. Zeev -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 02/03/15 08:39, Zeev Suraski wrote: One scenario I have in mind for this is validating $_GET information for a RESTful web service. Having potentially an infinite number of URIs that all point to the same resource isn't good: /users/1 /users/%201 /users/1%20 /users/%201%20 /users/%201%20%20 etc. In this case, I don't want to accept any leading or trailing spaces for the user ID. So I would therefore not be able to use an `int` type hint because its acceptance rules would be too lax. If spaces are not accepted in stringy ints, and I want to pass a stringy int that may have leading or trailing spaces in it, then I know I can simply apply a trim() to it before passing it into a function that's expecting only an int. This way, the usability of integer-only inputs (as string or ints) isn't compromised by the rules being too weak. Tom, First of all thanks for the feedback! I think that with leading/trailing spaces we could go either way. It boils down to the question of whether we want spaces to be explicitly trim()'d and have the rule-set more restrictive, or have the rule-set more permissive - preventing some use cases from using it and having to do manual validation instead. Based on the fact this has been a source of back forth changes (twitter.com/AndreaFaulds/status/570979956349665281), there's not going to be a one size fits all rule-set here. I think that the use cases where it will be harmless to ignore leading/trailing spaces would be more common than those where it's risky or undesired... Andrea's post highlights the fact that we did try a fix when PHP5 came out. What it fails to add is perhaps why the change was reverted? Still today there are people pressuring to have it restored? As this thread has shown. At the end of the day, this fine tuning of casting action has very little to do with the type hinting debate? Now IS the time to discuss the rules but not as part of some other RFC? It deserves it's own independent discussion as it SHOULD apply what ever happens over type hinting. Thomas's example in my book is where a number of things come into play? My first thought would be just where is this actually processed and so where is it trimmed? Additionally how about '001'? However it does highlight that a single 'int' hint is not going to satisfy everybody anyway? The thing perhaps to point out here is that we are looking in this case at a source that may well be using unicode and wonder if THAT may not be more of a problem here? -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Hey Zeev, Another change being considered and not yet in the RFC is re-allowing leading and trailing spaces for numeric strings (sorry Paddy.) I think that rejecting leading and trailing spaces for stringy ints is for the best. If I only want to accept an integer (in either int or string form), then by enabling spaces in that input, it would destroy the overall usability of it. One scenario I have in mind for this is validating $_GET information for a RESTful web service. Having potentially an infinite number of URIs that all point to the same resource isn't good: /users/1 /users/%201 /users/1%20 /users/%201%20 /users/%201%20%20 etc. In this case, I don't want to accept any leading or trailing spaces for the user ID. So I would therefore not be able to use an `int` type hint because its acceptance rules would be too lax. If spaces are not accepted in stringy ints, and I want to pass a stringy int that may have leading or trailing spaces in it, then I know I can simply apply a trim() to it before passing it into a function that's expecting only an int. This way, the usability of integer-only inputs (as string or ints) isn't compromised by the rules being too weak. The takeaway from this seems to be that the approach of tightening the existing rule-set and applying it to both internal functions and new user-land code is viable, and does not cause the sky to come down falling. Preliminary tests suggest it actually finds real potential problems. More tomorrow. Overall, a big +1 on this though! The type coercion rules at the moment are far too lax to be considered useful in most situations IMO. Zeev Thanks, Tom -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
De : Brian Moon [mailto:br...@moonspot.net]
I agree here with Benjamin. The thing I have wanted in user land for
years is to be able to build a user land function that worked the way
internal functions do in terms of type checking without having to build
my own type checking system. Having internal functions convert null to a
scalar and not having user land do the same creates an inconsistency.
For example, sometimes you want to wrap/extend an internal function (you
see it a lot with json* to do encoding checking). I can't reliably take
the same input to my user land wrapper that I can send to the internal
function. I would prefer to see the same rules apply to internal and
user land.
It would be better, yes. Unfortunately, accepting or refusing null to scalar in
both is difficult. If we disable null to scalar, we potentially generate a lot
of deprecation messages for existing code calling internal functions this way.
If we enable null to scalar, we create massive future inconsistencies now and
in the future. An example :
Function foo(): string {} // OK because returns null and null accepted as
string.
My opinion, but that's only mine, is that I would favor deprecating null to
scalar, even for internal functions. It would add deprecation messages but we'd
end up with a cleaner code.
Regards
François
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
De : Damien Tournoud [mailto:d...@damz.org] All those are due to a bug in substr(), that we see now only thanks to proper type identification. There is no reason for substr() to ever return a boolean. It really needs to be fix to always return a string. Sorry, not a bug. Documentation is clear. You get 'string|false' from substr() and send it to a function expecting 'string'. Most languages will fail on this. It worked in PHP because of implicit casting from bool to string. We just decided to deprecate this implicit cast. Now, we can discuss about substr(), whether it should always return string or not. But that's another subject. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 28/02/15 08:40, François Laupretre wrote: My opinion, but that's only mine, is that I would favor deprecating null to scalar, even for internal functions. It would add deprecation messages but we'd end up with a cleaner code. null is a very specific 'state' for anybody working with databases. It means that no value was found. Mapping that to a default value for future processing is a job for the application rather than the data packet as returned. It ONLY needs to overridden from a default or 0 in cases where the simple default is not required, but on the whole the default 'case' is ALL that is needed. That null is also false in situations where a bool is required is also natural. It's now having to go through all the code and manually add these quite natural casts which is the problem here. It's not a bug or mistake, it just flows in the whole process. -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
De : Benjamin Eberlei [mailto:kont...@beberlei.de] The funny thing is that the fix for this is: - substr($image, strrpos($image, \n) + 1) + (string)substr($image, strrpos($image, \n) + 1) Which is that sort of casting that is put forward as argument against the dual/strict mode. It is just one possible fix, but definitely not the one I would suggest, because too permissive. No controversy please, because both are valid. I would just prefer adding a test like 'if ($result===false) $result='';'. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
De : Benjamin Eberlei [mailto:kont...@beberlei.de] Interestingly functions like is_infinite(), is_dir(), is_file() all expect correct types like float or string here although from the naming convention is_something i would expect it says just false on *anything* else (which it almost does right now). Agreed. Must be modified. Even for 7.0 if it can be approved before freeze. All is_xxx functions must be checked and made more permissive on input arg (most, if not all, should probably accept any zval, minor BC break). Imho the problem is that the return values of php internal functions being string|false will lead to massive consecutive errors when passing this on to other internal unctions. Right. A side effect of the RFC is to encourage for an explicit check of false. May I say that this won't exactly lead to 'errors', as we are just deprecating usage of the implicit cast in such case. So it may lead to massive 'deprecated' messages but, apart from additional messages, behavior won't change. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
De : Damien Tournoud [mailto:d...@damz.org] But anyway, that's not even the point. The point is that return values that worked in a world of transparent type-jungling will not work in a world of stricter typing checks. Which means that we need: (1) To accept that for now casting FALSE to the empty string is the right thing to do; No. Add an explicit for false. We want programs to be cleaner, not encourage casting. until / unless: (2) We fixed all the common functions in the standard library so that they stop returning inconsistent types, in particular in the cases where it should not even be a debate what is the right thing to do (for substr, if the arguments point to an empty slice, return the empty string). That's history. Propose an RFC to fix substr() behavior (good luck :) Yes, we propose to deprecate potentially-problematic behaviors. That's not because we are running behind Anthony's RFC, as some may believe, but we sincerely think, at the end, it will bring more good than bad to PHP overall. You will have at least 5 years to fix your code. PHP is evolving. You like it or not. If you don't, you vote against the feature. That's democracy, which is not always the case for other languages. Can I say more ? François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 28/02/15 08:24, François Laupretre wrote: Sorry, not a bug. Documentation is clear. You get 'string|false' from substr() and send it to a function expecting 'string'. Most languages will fail on this. It worked in PHP because of implicit casting from bool to string. We just decided to deprecate this implicit cast. Now, we can discuss about substr(), whether it should always return string or not. But that's another subject. Isn't this the whole crux of the problem? The strict/coercive world fix for 'is there a string left' is to create an error which has to be handled, when workflow wise simply branching on the empty string is the correct action. I can see that some people need the empty string rather than the 'false' but THAT is the sort of thing that 'Coercive' should be handling rather than dictating that it is now an error. This is fundamental to the nature of PHP and dismissing a basic premiss 'another subject' is exactly what both 'camps' are currently doing. *I* want to maintain the ability to write code that runs in sequence, that is what a script language should do. It is NOT designed to be compiled, but processed and having to compile sections to create new functions such as exception returns which may never be used is wrong, just as trying to optimise something that may need the now optimized out element next time a bit of code is used is equally wrong. If you want the ultimate fastest performance just use C or one of the other fully compiled languages an live with the 'delay when you first run it'. Ultimately PHP works because it returns the type of object you need at the time ... fixed single types are not what PHP is ... and that is the type of PHP *I* want but I seem to be in an obsolete minority? I only need the current build of PHP7 because it puts back speed lost due to other changes but it is most definitely heading somewhere I don't want to be. -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Hi Christian, On Fri, Feb 27, 2015 at 5:15 PM, Christian Stoller stol...@leonex.de wrote: In this case FALSE is an expected result and it is intended. And as I said other languages are going the same way. Other languages are also doing something saner. It's not really an argument either way. Just because it makes more sense for you, it does not imply that it makes more sense for everybody. In my opinion it makes sense to return an error code if a function is called with invalid arguments. You probably haven't read the examples that I pasted. If you think that it makes sense to return an error code if the function is called with invalid arguments, it should be done consistently. The current behavior is just an absurdly inconsistent, it is really hard to find an argument in favor of it, other than the argument in favor of backward compatibility. Damien
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
This RFC trying to simpliy and cleanup the coercison rules, having two different conversion rules for NULL-scalar depending on userland or internal is counter-productive and bad. The behavior you describe as null being empty value is wide-spread in PHP userland code as well. I agree here with Benjamin. The thing I have wanted in user land for years is to be able to build a user land function that worked the way internal functions do in terms of type checking without having to build my own type checking system. Having internal functions convert null to a scalar and not having user land do the same creates an inconsistency. For example, sometimes you want to wrap/extend an internal function (you see it a lot with json* to do encoding checking). I can't reliably take the same input to my user land wrapper that I can send to the internal function. I would prefer to see the same rules apply to internal and user land. -- Brian. http://brian.moonspot.net/ -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 27/02/15 14:15, Benjamin Eberlei wrote: Imho the problem is that the return values of php internal functions being string|false will lead to massive consecutive errors when passing this on to other internal functions. This is perhaps the crux of my objection to both types of 'error checking' ... OK the return should be an empty string rather than false, but certainly one does not want an exception when nothing is returned. So much code IS based on doing one thing if there is a value and another when there is not, so if that is the problem people are claiming needs fixing perhaps that is what needs addressing? string|false is a core element of most of PHP? -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 27/02/15 16:46, Damien Tournoud wrote: You probably haven't read the examples that I pasted. If you think that it makes sense to return an error code if the function is called with invalid arguments, it should be done consistently. The current behavior is just an absurdly inconsistent, it is really hard to find an argument in favor of it, other than the argument in favor of backward compatibility. That some changes to the logic of substr have been made is a fact. Negative values were added to read back from the end of the string a function that has been played with over time, # It is NOT OK to start reading past the end of the string, no, no. Bad programmer. If there are no characters left ... the string is empty. # But it is OK to ask for a bigger slice than the length of the string. Yes ... length is the MOST it will return, if less are available then that is all you can get. # It is also OK to read past the begining of the string, why not. If the reading point does not cut characters then yes this is consistent. If the length requested was '2', then I would expect 'a' and false which I think was what the 'bug fix' for PHP5.2.2 was supposed to do, but it was reverted later in 5.2.7 # On the other hand, it is NOT OK to stop reading before the end of the string. Bad programmer. Take 2 characters off the string ... empty string. This may not be what YOU want substr to do and it would perhaps be useful to ADD additional checks so that 'false' is returned when it can't created a string because of the 'invalid arguments', but type hints makes no difference to part of the jigsaw. What happens is exactly what one would expect ... nothing left in the string - an empty string. Having then to check every time for a '0' string length got shortened to simply being able to check 'is there a string' yes/no, and if no you can do something else. There is nothing inconsistent ... -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 27/02/15 13:45, Benjamin Eberlei wrote: Drupal admin interface (across the all pages): One new E_DEPRECATED warning, which again seems to catch a real bug - stripslsahes() operating on a boolean. All those are due to a bug in substr(), that we see now only thanks to proper type identification. There is no reason for substr() to ever return a boolean. It really needs to be fix to always return a string. Yes, weird behavior that substr(, 2, 2); for example returns false. But changing thatis just another evil BC break. Now I don' think that 'weird' ... Although the correct return should perhaps be 'null', but it's long been practice that s there is no result we get 'false' so how any places will have a sanity check based on the 'false' return? -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On Fri, Feb 27, 2015 at 2:57 PM, Lester Caine les...@lsces.co.uk wrote: On 27/02/15 13:45, Benjamin Eberlei wrote: Drupal admin interface (across the all pages): One new E_DEPRECATED warning, which again seems to catch a real bug - stripslsahes() operating on a boolean. All those are due to a bug in substr(), that we see now only thanks to proper type identification. There is no reason for substr() to ever return a boolean. It really needs to be fix to always return a string. Yes, weird behavior that substr(, 2, 2); for example returns false. But changing thatis just another evil BC break. Now I don' think that 'weird' ... Although the correct return should perhaps be 'null', but it's long been practice that s there is no result we get 'false' so how any places will have a sanity check based on the 'false' return? As an example i ported PDepend to work with coercive typehints, this is the quick and dirty patch: https://gist.github.com/beberlei/6a5a6b65839d35bb27f0 In longer chains of string handling, i just cast everything to (string) that could potentially return false here. Interestingly functions like is_infinite(), is_dir(), is_file() all expect correct types like float or string here although from the naming convention is_something i would expect it says just false on *anything* else (which it almost does right now). The patch also shows that this will lead to weird behavior when the PHP API is actually wrong like DOMNode::cloneNode() expecting an integer and not a boolean as argument, even though the PHP Documentation says it should be a boolean. I can literally forsee this is the way this kind of code will be fixed. Imho the problem is that the return values of php internal functions being string|false will lead to massive consecutive errors when passing this on to other internal unctions. -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
From: Damien Tournoud [mailto:d...@damz.org], Sent: Friday, February 27, 2015 2:38 PM Hi Zeev, On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski z...@zend.com wrote: Drupal homepage: One new E_DEPRECATED warning, which seems to catch a real bug, or at least faulty looking code: $path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean false. return $path; Drupal admin interface (across the all pages): One new E_DEPRECATED warning, which again seems to catch a real bug - stripslsahes() operating on a boolean. All those are due to a bug in substr(), that we see now only thanks to proper type identification. There is no reason for substr() to ever return a boolean. It really needs to be fix to always return a string. Damien It is not a bug. FALSE as a return value of substr() is the identificator for an error (e.g. invalid arguments), as it is stated in the documentation: Returns the extracted part of string; or FALSE on failure, or an empty string. [1] This is an example which shows, that Zeevs RFC helps to find bugs in applications. Because here are given invalid arguments to the method. In other languages for example Java, you'll get an IndexOutOfBoundsException [2] if you are trying the same ;-) 1: http://php.net/manual/en/function.substr.php 2: http://docs.oracle.com/javase/7/docs/api/java/lang/String.html#substring%28int,%20int%29
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On Fri, Feb 27, 2015 at 2:37 PM, Damien Tournoud d...@damz.org wrote: Hi Zeev, On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski z...@zend.com wrote: Drupal homepage: One new E_DEPRECATED warning, which seems to catch a real bug, or at least faulty looking code: $path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean false. return $path; Drupal admin interface (across the all pages): One new E_DEPRECATED warning, which again seems to catch a real bug - stripslsahes() operating on a boolean. All those are due to a bug in substr(), that we see now only thanks to proper type identification. There is no reason for substr() to ever return a boolean. It really needs to be fix to always return a string. Yes, weird behavior that substr(, 2, 2); for example returns false. But changing that is just another evil BC break. Damien
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
-Original Message- From: Benjamin Eberlei [mailto:kont...@beberlei.de] Sent: Friday, February 27, 2015 1:19 PM To: Dmitry Stogov Cc: Zeev Suraski; PHP internals Subject: Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC On Fri, Feb 27, 2015 at 9:44 AM, Dmitry Stogov dmi...@zend.com wrote: I've added the link to the patch https://github.com/php/php-src/pull/1125/files Thanks! First, the necessary PHPUnit changes (dev-master) to avoid errors: https://gist.github.com/beberlei/8a33ae940829f1186da2 - Doctrine DBAL testsuite: 8 failures - Doctrine ORM: Crashes unrelated after half the tests, but has about 30-50% failures like Symfony2 - Symfony2 Testsuite: 6215 failures - Twig: Tests: 1108, Assertions: 1616, Errors: 125. Thanks for the tests! We'll look into those. Now probably many of the failures are related to few code paths that need fixing, however I have to find out that and will be a lot of work. But this is the good PHP code! Judging by the fact WordPress, Magento and Drupal appear a lot cleaner - you have to wonder..? :) But on a more serious note, I don't think we should assume just about anything from the test suite as reflecting on real world app behavior. I'm not familiar with the Symfony test suite (yet), but I do know that the Symfony skeleton app ran cleanly without a single warning. Is it possible it's testing a lot of things with garbage input, as you would in unit tests, as the our own PHP test suite does? Those account for most of the failures. Also take into account that a project with that many tests (and specifically, failures) is probably not going to have just one or two people responsible for it, but more. It's not that you personally would have to fix Symfony, there are at least a dozen if not dozen people that would help out. For untested or older PHP code (and yes there is alot out there) i now have to collect the errors in production to find all the occurances. This is nothing i can just do whenever I want, I need a process to collect and fix this over time. Now every company needs this process for every project they have out there. I wish that were true, but in reality, migration is painfully slow and judging by the numbers, doesn't happen - more often than happening. The 'good' companies have ~5yrs after PHP 7 comes out and before PHP 7.x will no longer be supported (and that's according to my hope for a fast PHP 8 timeline; it may be longer as you suggested). Would all projects migrate by that timeline? Let's make a simpler question - would all projects migrate to PHP 7 by 2017, the year 5.6 theoretically stops being supported? We all know the answer to that. The good companies, the security conscious ones that do keep up with new versions, will likely be willing to invest the effort of fixing the code (or hiring someone to do it for them), especially as it'll help them make their apps more robust/secure. And especially as - at least so far - it seems that real world apps have not even a handful of issues per page. All that said, I think we must understand the huge gap between the very high degree of test failures you're seeing in test suites, and the almost none we're seeing in real world apps. If it's similar to the PHP test suite, it's a non-issue, and even if it would take some work to fix the unit tests, it's well worth it. If it's legit, but as you're guessing, may just be several common paths of code that will do away with most of the failures - it's still worth it. Investing several days over the course of several years is great ROI for the value it brings. If, however, the test suite does find a multitude of many different issues - it may bring me to reconsider the RFC, as I told Anthony a few days ago - especially if the signal to noise ratio would be bad. After the results we've seen with real world apps I find it hard to believe, but it's certainly a possible scenario. At the end of the day, it boils down to how much real work do we think will be required to update real world apps out there. Let's also not set unreasonable bars for this change compared to other compatibility breakages we've done (and are stil doing) over the years - again, most of which didn't bring any tangible value to developers (which this change does, a lot). And the typical agency has hundrets/thousands of drupal, typo3, wordpress installations out there Correct, but would you have to patch each one separately? You'd have to install updated core for Drupal, Typo3, WP - which you commonly have to do anyway if you care about security. Then there'd be some work to get your common custom modules that you developed update - between the two of those, that should account for most of the code. It's not as if you're going to start with each deployment from scratch, is it? Anyway, thanks again for running the tests - we'll definitely need to understand that better. Thanks, Zeev -- PHP
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Hi Zeev, On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski z...@zend.com wrote: Drupal homepage: One new E_DEPRECATED warning, which seems to catch a real bug, or at least faulty looking code: $path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean false. return $path; Drupal admin interface (across the all pages): One new E_DEPRECATED warning, which again seems to catch a real bug - stripslsahes() operating on a boolean. All those are due to a bug in substr(), that we see now only thanks to proper type identification. There is no reason for substr() to ever return a boolean. It really needs to be fix to always return a string. Damien
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On Fri, Feb 27, 2015 at 2:37 PM, Damien Tournoud d...@damz.org wrote: Hi Zeev, On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski z...@zend.com wrote: Drupal homepage: One new E_DEPRECATED warning, which seems to catch a real bug, or at least faulty looking code: $path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean false. return $path; Drupal admin interface (across the all pages): One new E_DEPRECATED warning, which again seems to catch a real bug - stripslsahes() operating on a boolean. All those are due to a bug in substr(), that we see now only thanks to proper type identification. There is no reason for substr() to ever return a boolean. It really needs to be fix to always return a string. Damien This kind of code here exhibits the failure: https://github.com/pdepend/pdepend/blob/master/src/main/php/PDepend/Source/Language/PHP/PHPTokenizerInternal.php#L584 This is classic PHP style code where you rely on the implicit casting to make the algorithm work for you. if substr() here returns false, then the error is: 1767) PDepend\Source\Parser\UnstructuredCodeTest::testParserHandlesNonPhpCodeInFileProlog strlen() expects parameter 1 to be string, boolean given /home/benny/code/php/workspace/pdepend/src/main/php/PDepend/Source/Language/PHP/PHPTokenizerInternal.php:586 The funny thing is that the fix for this is: - substr($image, strrpos($image, \n) + 1) + (string)substr($image, strrpos($image, \n) + 1) Which is that sort of casting that is put forward as argument against the dual/strict mode.
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
From: Damien Tournoud [mailto:d...@damz.org], Sent: Friday, February 27, 2015 4:54 PM Hi Christian, On Fri, Feb 27, 2015 at 3:38 PM, Christian Stoller stol...@leonex.de wrote: It is not a bug. FALSE as a return value of substr() is the identificator for an error (e.g. invalid arguments), as it is stated in the documentation: [...] It is documented that way and it is not a bug are two very different things. That’s not true. Quoting Wikipedia A software bug is an error, flaw, failure, or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways. [1] In this case FALSE is an expected result and it is intended. And as I said other languages are going the same way. In that case, the semantics of `substr()` are just wrong. It makes a lot more sense for a sub-string function to silently allow reading before the beginning and past the end of a string. Just because it makes more sense for you, it does not imply that it makes more sense for everybody. In my opinion it makes sense to return an error code if a function is called with invalid arguments. This behavior is unpredictable, and as a consequence using `substr()` properly would require a lot of painful and unnecessary up-front checks. I agree with you, here. With the coercive patch this will lead to More checks. But that's different from it is a bug ;-) 1: http://en.wikipedia.org/wiki/Software_bug
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Cristian, All those are due to a bug in substr(), that we see now only thanks to proper type identification. There is no reason for substr() to ever return a boolean. It really needs to be fix to always return a string. Damien It is not a bug. FALSE as a return value of substr() is the identificator for an error (e.g. invalid arguments), as it is stated in the documentation: Returns the extracted part of string; or FALSE on failure, or an empty string. [1] This is an example which shows, that Zeevs RFC helps to find bugs in applications. Because here are given invalid arguments to the method. In other languages for example Java, you'll get an IndexOutOfBoundsException [2] if you are trying the same ;-) Well, it depends on your definition of bug. If you look at PHPUnit's TestSuite::isTestMethod method, you'll see this: $doc_comment = $method-getDocComment(); return strpos($doc_comment, '@test') !== false || strpos($doc_comment, '@scenario') !== false; Now, with the coercive mode (and my strict mode), that will error if the method does not have a docblock. The reason is that `$method-getDocComment()` returns false if one does not exist. Is that a bug? Well, semantically, maybe. But from the application level, no. Passing the boolean result to `strpos` directly ensures that the result will be `bool(false)`. So the overall method behaves as expected returning true in the correct context (it's a test method) and false in incorrect contexts (it's not). So did the coercive mode find a bug? Not really. It found something that may have worked in a way you didn't expect it to, but it wasn't a bug (at least to the application). Now, if you care about types and type changes, then yes, that is a bug. And that's why the dual-mode RFC gives you the choice. You can choose to care about types or not. Not some weird hybrid where you have to care even if the code is practically correct... Anthony -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Hi Christian, On Fri, Feb 27, 2015 at 3:38 PM, Christian Stoller stol...@leonex.de wrote: It is not a bug. FALSE as a return value of substr() is the identificator for an error (e.g. invalid arguments), as it is stated in the documentation: [...] It is documented that way and it is not a bug are two very different things. In that case, the semantics of `substr()` are just wrong. It makes a lot more sense for a sub-string function to silently allow reading before the beginning and past the end of a string. Moreover, the *conditions* under which `substr()` returns FALSE looks completely arbitrary, see http://3v4l.org/gtFjk -- in a nutshell: so it is OK to read past the beginning, it is OK to try to get more characters than the string has, but it is NOT OK to try to start reading past the end? This behavior is unpredictable, and as a consequence using `substr()` properly would require a lot of painful and unnecessary up-front checks. It traditionally did not matter, because of the fluid scalar casts. But if we want now to introduce stricter casts, we are going to have to tackle this problem, we cannot just dismiss it as a programming error. Damien
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Hi Lester, On Fri, Feb 27, 2015 at 6:51 PM, Lester Caine les...@lsces.co.uk wrote: This may not be what YOU want substr to do and it would perhaps be useful to ADD additional checks so that 'false' is returned when it can't created a string because of the 'invalid arguments', but type hints makes no difference to part of the jigsaw. What happens is exactly what one would expect ... nothing left in the string - an empty string. Having then to check every time for a '0' string length got shortened to simply being able to check 'is there a string' yes/no, and if no you can do something else. There is nothing inconsistent ... Please, read the examples again, the current behavior is nothing but inconsistent: substr(a, 1) = FALSE substr(a, -300) = But anyway, that's not even the point. The point is that return values that worked in a world of transparent type-jungling will not work in a world of stricter typing checks. Which means that we need: (1) To accept that for now casting FALSE to the empty string is the right thing to do; until / unless: (2) We fixed all the common functions in the standard library so that they stop returning inconsistent types, in particular in the cases where it should not even be a debate what is the right thing to do (for substr, if the arguments point to an empty slice, return the empty string). Damien
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 27/02/15 20:28, Damien Tournoud wrote: Hi Lester, On Fri, Feb 27, 2015 at 6:51 PM, Lester Caine les...@lsces.co.uk mailto:les...@lsces.co.uk wrote: This may not be what YOU want substr to do and it would perhaps be useful to ADD additional checks so that 'false' is returned when it can't created a string because of the 'invalid arguments', but type hints makes no difference to part of the jigsaw. What happens is exactly what one would expect ... nothing left in the string - an empty string. Having then to check every time for a '0' string length got shortened to simply being able to check 'is there a string' yes/no, and if no you can do something else. There is nothing inconsistent ... Please, read the examples again, the current behavior is nothing but inconsistent: substr(a, 1) = FALSE substr(a, -300) = ? That was the case prior to PHP5.2.1 The fixes in 5.2.2 were not commonly accepted but give false for both, but 5.2.7 and later give false and a which is what was the preferred result at the time. Unless we are seeing something different, I'm only seeing a or false for a current output of all your examples. But anyway, that's not even the point. The point is that return values that worked in a world of transparent type-jungling will not work in a world of stricter typing checks. Which means that we need: (1) To accept that for now casting FALSE to the empty string is the right thing to do; until / unless: (2) We fixed all the common functions in the standard library so that they stop returning inconsistent types, in particular in the cases where it should not even be a debate what is the right thing to do (for substr, if the arguments point to an empty slice, return the empty string). That side I totally agree with. The 'fixes' posted so far such as adding (string) or replacing false by 0 should be enough to prevent adoption, and personally I find the strict option just as problematic. -- 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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Zeev,
On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski z...@zend.com wrote:
All,
We've been working in the last few days to test and tune the Coercive STH
patch. I think the results are quite nice, and surprisingly better than
one might have expected.
Can we try the patch ourselves? I would love to run it against some
libraries as well.
Before diving into the results, we did update the RFC
(wiki.php.net/rfc/coercive_sth) - with the most notable difference being
allowing NULL-scalar conversions, for two reasons - it's not uncommon for
code to use 'null' as a way to denote an empty optional parameter to for
internal functions, and many internal functions seem to rely on that
behavior themselves. It should be possible to alter this behavior in the
future by changing the internal functions to explicitly handle NULL inputs
as 'please use the default value' - but it's outside the scope of the RFC.
This RFC trying to simpliy and cleanup the coercison rules, having two
different conversion rules for NULL-scalar
depending on userland or internal is counter-productive and bad. The
behavior you describe as null being
empty value is wide-spread in PHP userland code as well.
In addition, coercion from scalar to boolean is now limited only to
integer - which seems to be the most popular use case; Beforehand,
coercion was also allowed from float and string - but based on feedback
from Mike, we're reconsidering accepting strings again.
Another change being considered and not yet in the RFC is re-allowing
leading and trailing spaces for numeric strings (sorry Paddy.)
I agree with Pierre here, it would be super helpful if we had a log in the
RFC of the actual changes that will be happening.
As in Francois original patch this seems to be a game of having 20 changes
and then picking which ones to do and which not.
Now to the tests we ran. The goal was to see what kind of effect the
changes to the internal function APIs had on real world apps with large
codebase. The test methodology was done by placing a debugger breakpoint
on zend_error, to ensure no error gets lost in the ether of settings or
callbacks. Here are the results:
Drupal homepage: One new E_DEPRECATED warning, which seems to catch a
real bug, or at least faulty looking code:
$path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean
false.
return $path;
Drupal admin interface (across the all pages): One new E_DEPRECATED
warning, which again seems to catch a real bug - stripslsahes() operating
on a boolean.
Magento homepage (w/ Magento's huge sample DB): One new E_DEPRECATED
warning, again, seems to be catching a real bug of 'false' being fed as
argument 1 of in json_decode() - which is expecting a string full of json
there.
WordPress homepage: One new E_DEPRECATED warning, again, seems to be
catching a real bug of 'false' being fed as argument 1 of substr().
Zend Framework 2 skeleton app: Zero new E_DEPRECATED warnings.
Symfony ACME app: Zero new E_DEPRECATED warnings (across the app).
I was expecting this, because the rule changes are mostly in regard to not
accepting
invalid input, so what you need to test is all the edge cases.
Say I rely on a validation in count() somewhere in the code to implicitly
validate its an array:
if (count($_GET['filters'])) {
echo Filtering my query;
}
This would work in the happy path case, because i have a filter set. But
maybe
there is some invalid state i can get into and only then the E_DEPRECATED
is produced.
The homepages of popular systems being the essential happy path for a
project, I wouldnt expect many errors to occur.
As I'm sure you know, the above tests invoke a huge number of lines of
code in total, handling filesystem ops, database ops and all sorts of
other things. This is much of the mini test suite that we use to test
PHP's performance and compatibility (e.g. phpng, now PHP 7). So while
this isn't proof that code in the wild isn't going to have more issues -
it's a pretty good initial indication regarding the level of 'breakage' we
can expect. I'm putting breakage in quotes, as people would have several
years to fix these few issues, before the E_DEPRECATED becomes an error
(or an exception, if the engine exceptions RFC passes).
In terms of the test suite (.phpts), the changes cause approximately 700
extra tests to fail out of 13,700, in comparison to w/o the patch.
However, even though I didn't have a chance to go over all of them, it
seems that the vast majority of the failed tests are tests that were
intentionally designed to cover the exact parameter passing behavior,
rather than real likely real world code pieces. A huge number of the
internal functions have this in their test suites:
$variation = array(
'float 10.5' = 10.5,
'float -10.5' = -10.5,
'float 12.3456789000e10' = 12.3456789000e10,
'float -12.3456789000e10' = -12.3456789000e10,
'float .5' = .5,
);
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Hi Lester,
On Fri, Feb 27, 2015 at 9:53 PM, Lester Caine les...@lsces.co.uk wrote:
Please, read the examples again, the current behavior is nothing but
inconsistent:
substr(a, 1) = FALSE
substr(a, -300) =
? That was the case prior to PHP5.2.1
The fixes in 5.2.2 were not commonly accepted but give false for both,
but 5.2.7 and later give false and a which is what was the preferred
result at the time. Unless we are seeing something different, I'm only
seeing a or false for a current output of all your examples.
I meant a, but you are right, it's a bit less inconsistent than I
thought. The current behavior could be defined as if the resulting slice
is so that (start index = end index) and (either start index or end index
is in bound) return the slice, else return FALSE, which is not very useful
but not so bad anyway.
The only real annoyance is that the check is strict on the right bound, so
that:
substr(abcd, 5) = FALSE
while:
substr(abcd, -10, -4) =
That explains one of the E_DEPRECATED triggered by Drupal 7, because when
you want to remove a prefix from a string, you often do:
if (substr($str, 0, strlen($prefix)) {
$str = substr($str, strlen($prefix))
}
But this currently returns FALSE when $str == $prefix.
(Obviously, the most useful behavior would be to return a string in all
cases, like for example, Python.)
Damien
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
I've added the link to the patch
https://github.com/php/php-src/pull/1125/files
Thanks. Dmitry.
On Fri, Feb 27, 2015 at 11:03 AM, Benjamin Eberlei kont...@beberlei.de
wrote:
Zeev,
On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski z...@zend.com wrote:
All,
We've been working in the last few days to test and tune the Coercive STH
patch. I think the results are quite nice, and surprisingly better than
one might have expected.
Can we try the patch ourselves? I would love to run it against some
libraries as well.
Before diving into the results, we did update the RFC
(wiki.php.net/rfc/coercive_sth) - with the most notable difference being
allowing NULL-scalar conversions, for two reasons - it's not uncommon
for
code to use 'null' as a way to denote an empty optional parameter to for
internal functions, and many internal functions seem to rely on that
behavior themselves. It should be possible to alter this behavior in the
future by changing the internal functions to explicitly handle NULL
inputs
as 'please use the default value' - but it's outside the scope of the
RFC.
This RFC trying to simpliy and cleanup the coercison rules, having two
different conversion rules for NULL-scalar
depending on userland or internal is counter-productive and bad. The
behavior you describe as null being
empty value is wide-spread in PHP userland code as well.
In addition, coercion from scalar to boolean is now limited only to
integer - which seems to be the most popular use case; Beforehand,
coercion was also allowed from float and string - but based on feedback
from Mike, we're reconsidering accepting strings again.
Another change being considered and not yet in the RFC is re-allowing
leading and trailing spaces for numeric strings (sorry Paddy.)
I agree with Pierre here, it would be super helpful if we had a log in the
RFC of the actual changes that will be happening.
As in Francois original patch this seems to be a game of having 20 changes
and then picking which ones to do and which not.
Now to the tests we ran. The goal was to see what kind of effect the
changes to the internal function APIs had on real world apps with large
codebase. The test methodology was done by placing a debugger breakpoint
on zend_error, to ensure no error gets lost in the ether of settings or
callbacks. Here are the results:
Drupal homepage: One new E_DEPRECATED warning, which seems to catch a
real bug, or at least faulty looking code:
$path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean
false.
return $path;
Drupal admin interface (across the all pages): One new E_DEPRECATED
warning, which again seems to catch a real bug - stripslsahes() operating
on a boolean.
Magento homepage (w/ Magento's huge sample DB): One new E_DEPRECATED
warning, again, seems to be catching a real bug of 'false' being fed as
argument 1 of in json_decode() - which is expecting a string full of json
there.
WordPress homepage: One new E_DEPRECATED warning, again, seems to be
catching a real bug of 'false' being fed as argument 1 of substr().
Zend Framework 2 skeleton app: Zero new E_DEPRECATED warnings.
Symfony ACME app: Zero new E_DEPRECATED warnings (across the app).
I was expecting this, because the rule changes are mostly in regard to not
accepting
invalid input, so what you need to test is all the edge cases.
Say I rely on a validation in count() somewhere in the code to implicitly
validate its an array:
if (count($_GET['filters'])) {
echo Filtering my query;
}
This would work in the happy path case, because i have a filter set. But
maybe
there is some invalid state i can get into and only then the E_DEPRECATED
is produced.
The homepages of popular systems being the essential happy path for a
project, I wouldnt expect many errors to occur.
As I'm sure you know, the above tests invoke a huge number of lines of
code in total, handling filesystem ops, database ops and all sorts of
other things. This is much of the mini test suite that we use to test
PHP's performance and compatibility (e.g. phpng, now PHP 7). So while
this isn't proof that code in the wild isn't going to have more issues -
it's a pretty good initial indication regarding the level of 'breakage'
we
can expect. I'm putting breakage in quotes, as people would have several
years to fix these few issues, before the E_DEPRECATED becomes an error
(or an exception, if the engine exceptions RFC passes).
In terms of the test suite (.phpts), the changes cause approximately 700
extra tests to fail out of 13,700, in comparison to w/o the patch.
However, even though I didn't have a chance to go over all of them, it
seems that the vast majority of the failed tests are tests that were
intentionally designed to cover the exact parameter passing behavior,
rather than real likely real world code
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On Fri, Feb 27, 2015 at 9:44 AM, Dmitry Stogov dmi...@zend.com wrote:
I've added the link to the patch
https://github.com/php/php-src/pull/1125/files
Thanks!
First, the necessary PHPUnit changes (dev-master) to avoid errors:
https://gist.github.com/beberlei/8a33ae940829f1186da2
- Doctrine DBAL testsuite: 8 failures
- Doctrine ORM: Crashes unrelated after half the tests, but has about
30-50% failures like Symfony2
- Symfony2 Testsuite: 6215 failures
- Twig: Tests: 1108, Assertions: 1616, Errors: 125.
Now probably many of the failures are related to few code paths that need
fixing, however I have to find out that and will be a lot of work. But this
is the good PHP code!
For untested or older PHP code (and yes there is alot out there) i now have
to collect the errors in production to find all the occurances. This is
nothing i can just do whenever I want, I need a process to collect and fix
this over time. Now every company needs this process for every project they
have out there. And the typical agency has hundrets/thousands of drupal,
typo3, wordpress installations out there.
Thanks. Dmitry.
On Fri, Feb 27, 2015 at 11:03 AM, Benjamin Eberlei kont...@beberlei.de
wrote:
Zeev,
On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski z...@zend.com wrote:
All,
We've been working in the last few days to test and tune the Coercive
STH
patch. I think the results are quite nice, and surprisingly better than
one might have expected.
Can we try the patch ourselves? I would love to run it against some
libraries as well.
Before diving into the results, we did update the RFC
(wiki.php.net/rfc/coercive_sth) - with the most notable difference
being
allowing NULL-scalar conversions, for two reasons - it's not uncommon
for
code to use 'null' as a way to denote an empty optional parameter to for
internal functions, and many internal functions seem to rely on that
behavior themselves. It should be possible to alter this behavior in
the
future by changing the internal functions to explicitly handle NULL
inputs
as 'please use the default value' - but it's outside the scope of the
RFC.
This RFC trying to simpliy and cleanup the coercison rules, having two
different conversion rules for NULL-scalar
depending on userland or internal is counter-productive and bad. The
behavior you describe as null being
empty value is wide-spread in PHP userland code as well.
In addition, coercion from scalar to boolean is now limited only to
integer - which seems to be the most popular use case; Beforehand,
coercion was also allowed from float and string - but based on feedback
from Mike, we're reconsidering accepting strings again.
Another change being considered and not yet in the RFC is re-allowing
leading and trailing spaces for numeric strings (sorry Paddy.)
I agree with Pierre here, it would be super helpful if we had a log in the
RFC of the actual changes that will be happening.
As in Francois original patch this seems to be a game of having 20 changes
and then picking which ones to do and which not.
Now to the tests we ran. The goal was to see what kind of effect the
changes to the internal function APIs had on real world apps with large
codebase. The test methodology was done by placing a debugger
breakpoint
on zend_error, to ensure no error gets lost in the ether of settings or
callbacks. Here are the results:
Drupal homepage: One new E_DEPRECATED warning, which seems to catch a
real bug, or at least faulty looking code:
$path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean
false.
return $path;
Drupal admin interface (across the all pages): One new E_DEPRECATED
warning, which again seems to catch a real bug - stripslsahes()
operating
on a boolean.
Magento homepage (w/ Magento's huge sample DB): One new E_DEPRECATED
warning, again, seems to be catching a real bug of 'false' being fed as
argument 1 of in json_decode() - which is expecting a string full of
json
there.
WordPress homepage: One new E_DEPRECATED warning, again, seems to be
catching a real bug of 'false' being fed as argument 1 of substr().
Zend Framework 2 skeleton app: Zero new E_DEPRECATED warnings.
Symfony ACME app: Zero new E_DEPRECATED warnings (across the app).
I was expecting this, because the rule changes are mostly in regard to not
accepting
invalid input, so what you need to test is all the edge cases.
Say I rely on a validation in count() somewhere in the code to implicitly
validate its an array:
if (count($_GET['filters'])) {
echo Filtering my query;
}
This would work in the happy path case, because i have a filter set. But
maybe
there is some invalid state i can get into and only then the E_DEPRECATED
is produced.
The homepages of popular systems being the essential happy path for a
project, I wouldnt expect many errors to occur.
As I'm sure you know, the
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 2/27/15 10:15 AM, Christian Stoller wrote: From: Damien Tournoud [mailto:d...@damz.org], Sent: Friday, February 27, 2015 4:54 PM Hi Christian, On Fri, Feb 27, 2015 at 3:38 PM, Christian Stoller stol...@leonex.de wrote: It is not a bug. FALSE as a return value of substr() is the identificator for an error (e.g. invalid arguments), as it is stated in the documentation: [...] It is documented that way and it is not a bug are two very different things. That’s not true. Quoting Wikipedia A software bug is an error, flaw, failure, or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways. [1] In this case FALSE is an expected result and it is intended. And as I said other languages are going the same way. Q: How many MicroSoft engineers does it take to change a light bulb? A: None. Bill Gates will just redefine Darkness(TM) as the new industry standard. --Larry Garfield (Sorry, it was just such an obvious opening...) -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On 27/02/15 21:14, Damien Tournoud wrote:
Hi Lester,
On Fri, Feb 27, 2015 at 9:53 PM, Lester Caine les...@lsces.co.uk
mailto:les...@lsces.co.uk wrote:
Please, read the examples again, the current behavior is nothing but
inconsistent:
substr(a, 1) = FALSE
substr(a, -300) =
? That was the case prior to PHP5.2.1
The fixes in 5.2.2 were not commonly accepted but give false for both,
but 5.2.7 and later give false and a which is what was the preferred
result at the time. Unless we are seeing something different, I'm only
seeing a or false for a current output of all your examples.
I meant a, but you are right, it's a bit less inconsistent than I
thought. The current behavior could be defined as if the resulting
slice is so that (start index = end index) and (either start index or
end index is in bound) return the slice, else return FALSE, which is
not very useful but not so bad anyway.
The only real annoyance is that the check is strict on the right bound,
so that:
substr(abcd, 5) = FALSE
while:
substr(abcd, -10, -4) =
This is the where the -ve logic stuff was changed. If you try -5 rather
than -4 things swap around versions wise. The 'problem' is where the
actual buffer length goes to 0 and then -ve. There still seem to be a
few edge cases that are not correctly caught, but then currently and
false default to false anyway so there is ONLY a problem if one DOES
remove the weak casting.
That explains one of the E_DEPRECATED triggered by Drupal 7, because
when you want to remove a prefix from a string, you often do:
if (substr($str, 0, strlen($prefix)) {
$str = substr($str, strlen($prefix))
}
But this currently returns FALSE when $str == $prefix.
(Obviously, the most useful behavior would be to return a string in all
cases, like for example, Python.)
If you only want strings then OK, but in that case how would you rework
the drupal problem anyway? Although I'd not start from that if statement
anyway if there is a chance that the $str and $prefix ARE the same
string? The current if statement is what is wrong not the second substr.
if ( strlen($str) strlen($prefix) ) {
$str = substr($str, strlen($prefix))
} else { // nothing left }
But how do you know that the string contains the prefix? Yes the legacy
code base needs tidying up and is full of bugs, but we often just need
to tidy up the logic ...
--
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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On Fri, Feb 27, 2015 at 12:18 PM, Benjamin Eberlei kont...@beberlei.de wrote: On Fri, Feb 27, 2015 at 9:44 AM, Dmitry Stogov dmi...@zend.com wrote: I've added the link to the patch https://github.com/php/php-src/pull/1125/files Thanks! First, the necessary PHPUnit changes (dev-master) to avoid errors: https://gist.github.com/beberlei/8a33ae940829f1186da2 - Doctrine DBAL testsuite: 8 failures - Doctrine ORM: Crashes unrelated after half the tests, but has about 30-50% failures like Symfony2 - Symfony2 Testsuite: 6215 failures - Twig: Tests: 1108, Assertions: 1616, Errors: 125. Now probably many of the failures are related to few code paths that need fixing, however I have to find out that and will be a lot of work. But this is the good PHP code! For untested or older PHP code (and yes there is alot out there) i now have to collect the errors in production to find all the occurances. This is nothing i can just do whenever I want, I need a process to collect and fix this over time. Now every company needs this process for every project they have out there. And the typical agency has hundrets/thousands of drupal, typo3, wordpress installations out there. I used the following configure line and then /usr/local/php7/lib/php.ini ./configure --prefix=/usr/local/php7 --with-mysql=mysqlnd --with-mysqli=mysqlnd --with-tidy=/usr --with-curl=/usr --with-curlwrappers --with-zlib-dir=/usr --enable-mbstring --with-xpm-dir=/usr --enable-pdo=shared --with-pdo-mysql=shared,mysqlnd --without-sqlite --with-pdo-sqlite=shared,/usr --with-xsl=/usr --with-xmlrpc --with-iconv-dir=/usr --with-snmp=/usr --enable-exif --enable-calendar --with-bz2=/usr --with-mcrypt=/usr --with-gd --with-jpeg-dir=/usr --with-png-dir=/usr --with-zlib-dir=/usr --with-freetype-dir=/usr --enable-mbstring --enable-zip --with-pear --enable-soap --with-gd --enable-intl --with-bz2=/usr --enable-sockets --with-sqlite3=shared --enable-pcntl max_execution_time=600 memory_limit=128M error_reporting=-1 display_errors=1 log_errors=0 user_ini.filename= realpath_cache_size=2M cgi.check_shebang_line=0 zend_extension=opcache.so opcache.enable_cli=1 opcache.save_comments=1 opcache.fast_shutdown=1 opcache.validate_timestamps=1 opcache.revalidate_freq=60 opcache.use_cwd=1 opcache.max_accelerated_files=10 opcache.max_wasted_percentage=5 opcache.memory_consumption=128 opcache.consistency_checks=0 date.timezone=Europe/Berlin extension=pdo.so extension=pdo_sqlite.so extension=pdo_mysql.so extension=sqlite3.so
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Hi Pierre, De : Pierre Joye [mailto:pierre@gmail.com] Also I would really like a clear table describing in details what will be changed, to compare how it works now and how it work later. The RFC now contains a table listing the changes compared to current PHP 5 rules. Enjoy it. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
How many deprecations do you get running the ZF2 and Symfony testsuites? None, but it may have to do with the fact I haven't run them yet :) Zeev -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On Thu, Feb 26, 2015 at 3:57 PM, Zeev Suraski z...@zend.com wrote: Drupal homepage: One new E_DEPRECATED warning, which seems to catch a real bug, or at least faulty looking code: $path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean false. return $path; So far nobody answered this question but Francois (tried). You keep using this E_DEPRECATED message as a safety net to catch possible bad things. What's about other changes that may not match the current casting rules (or lack of) and will be different with this RFC? Those considered as Yes. This is what I worry about, even if minimal these cases are pains to track down and fix. Also I would really like a clear table describing in details what will be changed, to compare how it works now and how it work later. This is something that is crystal clear with the dual mode RFC, nothing changes by defaullt, and strict rules if strict mode is enabled for a file. As you would expect, I will vote no as it is proposed now. -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
On Fri, Feb 27, 2015 at 12:57 AM, Zeev Suraski z...@zend.com wrote:
All,
We've been working in the last few days to test and tune the Coercive STH
patch. I think the results are quite nice, and surprisingly better than
one might have expected.
Before diving into the results, we did update the RFC
(wiki.php.net/rfc/coercive_sth) - with the most notable difference being
allowing NULL-scalar conversions, for two reasons - it's not uncommon for
code to use 'null' as a way to denote an empty optional parameter to for
internal functions, and many internal functions seem to rely on that
behavior themselves. It should be possible to alter this behavior in the
future by changing the internal functions to explicitly handle NULL inputs
as 'please use the default value' - but it's outside the scope of the RFC.
In addition, coercion from scalar to boolean is now limited only to
integer - which seems to be the most popular use case; Beforehand,
coercion was also allowed from float and string - but based on feedback
from Mike, we're reconsidering accepting strings again.
Another change being considered and not yet in the RFC is re-allowing
leading and trailing spaces for numeric strings (sorry Paddy.)
Now to the tests we ran. The goal was to see what kind of effect the
changes to the internal function APIs had on real world apps with large
codebase. The test methodology was done by placing a debugger breakpoint
on zend_error, to ensure no error gets lost in the ether of settings or
callbacks. Here are the results:
Drupal homepage: One new E_DEPRECATED warning, which seems to catch a
real bug, or at least faulty looking code:
$path = trim($path, '/'); // raises E_DEPRECATED, as $path is boolean
false.
return $path;
Drupal admin interface (across the all pages): One new E_DEPRECATED
warning, which again seems to catch a real bug - stripslsahes() operating
on a boolean.
Magento homepage (w/ Magento's huge sample DB): One new E_DEPRECATED
warning, again, seems to be catching a real bug of 'false' being fed as
argument 1 of in json_decode() - which is expecting a string full of json
there.
WordPress homepage: One new E_DEPRECATED warning, again, seems to be
catching a real bug of 'false' being fed as argument 1 of substr().
Zend Framework 2 skeleton app: Zero new E_DEPRECATED warnings.
Symfony ACME app: Zero new E_DEPRECATED warnings (across the app).
As I'm sure you know, the above tests invoke a huge number of lines of
code in total, handling filesystem ops, database ops and all sorts of
other things. This is much of the mini test suite that we use to test
PHP's performance and compatibility (e.g. phpng, now PHP 7). So while
this isn't proof that code in the wild isn't going to have more issues -
it's a pretty good initial indication regarding the level of 'breakage' we
can expect. I'm putting breakage in quotes, as people would have several
years to fix these few issues, before the E_DEPRECATED becomes an error
(or an exception, if the engine exceptions RFC passes).
In terms of the test suite (.phpts), the changes cause approximately 700
extra tests to fail out of 13,700, in comparison to w/o the patch.
However, even though I didn't have a chance to go over all of them, it
seems that the vast majority of the failed tests are tests that were
intentionally designed to cover the exact parameter passing behavior,
rather than real likely real world code pieces. A huge number of the
internal functions have this in their test suites:
$variation = array(
'float 10.5' = 10.5,
'float -10.5' = -10.5,
'float 12.3456789000e10' = 12.3456789000e10,
'float -12.3456789000e10' = -12.3456789000e10,
'float .5' = .5,
);
foreach ( $variation as $var ) {
var_dump(readgzfile( $filename, $var ) );
}
Which clearly isn't a very likely input to the size argument of
readgzfile(). All of these now trigger E_DEPRECATED warnings since we no
longer allow float-int conversions if there's no data loss. For some
reason (perhaps a good one, not sure), we have virtually identical pieces
of code for dozens if not hundreds of internal functions that expect
integers - and these types of failures account for the (looks like vast)
majority of phpt failures. Quoting Andrea from a couple of days ago on
this very topic, to be fair, most of PHP's test suite basically just
tests zpp's behavior, which appears to be absolutely true. The
real-world app tests are probably a much better indicator of the kind of
behaviors our users are going to see than our test suite is.
The takeaway from this seems to be that the approach of tightening the
existing rule-set and applying it to both internal functions and new
user-land code is viable, and does not cause the sky to come down falling.
Preliminary tests suggest it actually finds real potential problems.
More tomorrow.
Zeev
--
PHP Internals - PHP Runtime Development Mailing List
Re: [PHP-DEV] Coercive STH - some real world tests and updated RFC
Hi Zeev, On 26 February 2015 at 23:57, Zeev Suraski z...@zend.com wrote: Another change being considered and not yet in the RFC is re-allowing leading and trailing spaces for numeric strings (sorry Paddy.) Not to worry. I know where Github keeps the servers ;). There are far worse things than spaces. Paddy -- Pádraic Brady http://blog.astrumfutura.com http://www.survivethedeepend.com -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
RE: [PHP-DEV] Coercive STH - some real world tests and updated RFC
De : Pierre Joye [mailto:pierre@gmail.com] So far nobody answered this question but Francois (tried). You keep using this E_DEPRECATED message as a safety net to catch possible bad things. Yes, even if we don't really 'catch' things, because we just raise E_DEPRECATED and, then, continue execution exactly as before. Actually, it is more an 'alert' net than a safety net, as its role is to alert developers that they have a potential issue to solve during the next 5 or 10 years. In most cases, it is a real bug they'd better fix before 5 years ;) What's about other changes that may not match the current casting rules (or lack of) and will be different with this RFC? Those considered as Yes. I now understand your previous post. There's absolutely no change in the way things are converted. Every conversion works *exactly* the same as in PHP 5. The only additional behavior we introduce is raising some E_DEPRECATED in some cases. But, after raising E_DEPRECATED, we still do the work ! So, if you turn E_DEPRECATED off, the behavior is exactly the same as PHP 5. Even when E_DEPRECATED will be turned to fatal error in the future, the ruleset will remain a pure subset of the cases supported in PHP 5, failing for more cases, but with no change in the way conversion are performed. I think it is not clear enough in the RFC, explaining your misconception. Actually, I must admit this is not *perfectly* true. There is one conversion I would like to modify. If we reintroduce getting bool from string, the current case for false is 0 or empty string only. For consistency reasons, I would like to extend 0 to any string that would convert to a null numeric value (also considering as false 0.0,0,...) but I hope you will consider that's a marginal change and the risk is low. But this is just a suggestion. Even for this, if some prefer to stick with the current behavior, it will be preserved and kept for a future RFC. Also I would really like a clear table describing in details what will be changed, to compare how it works now and how it work later. You're right. I had planned to write two tables. One with the changes and one with the final situation. But I didn't have enough time for both. I will add the 'changes' table in the next hours. Regards François -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
