| Both sides previous revisionPrevious revisionNext revision | Previous revisionNext revisionBoth sides next revision |
| rfc:notice-for-non-valid-array-container [2016/08/10 20:20] – bp1222 | rfc:notice-for-non-valid-array-container [2016/08/31 18:09] – bp1222 |
|---|
| ====== PHP RFC: E_NOTICE for invalid container read array-access ====== | ====== PHP RFC: E_WARNING for invalid container read array-access ====== |
| * Version: 0.2 | * Version: 1.0 |
| * Date: 2016-08-10 | * Date: 2016-08-16 |
| * Author: David Walker (dave@mudsite.com) | * Author: David Walker (dave@mudsite.com) |
| * Status: Under Discusson | * Status: Voting |
| * First Published at: http://wiki.php.net/rfc/notice-for-non-valid-array-container | * First Published at: http://wiki.php.net/rfc/notice-for-non-valid-array-container |
| |
| ===== Introduction ===== | ===== Introduction ===== |
| PHP current array-access system works very well when attempting to access containers of type array, reference(to an array), and objects that implement ArrayAccess. However, accessing elements not of those types results in a NULL value returned without any alert as to why it's null. This feature has been requested in [[https://bugs.php.net/bug.php?id=37676|37676]] and duplicated in at least [[https://bugs.php.net/bug.php?id=39915|39915]] and [[https://bugs.php.net/bug.php?id=72636|72636]]. PHP correctly returns a notice when you attempt to use an offset that is of an invalid type, PHP should also provide a notice when a container is of an invalid type. | PHP current array-access system works very well when attempting to access containers of type array, reference(to an array), and objects that implement ArrayAccess. However, accessing elements not of those types results in a NULL value returned without any alert as to why it's null. This feature has been requested in [[https://bugs.php.net/bug.php?id=37676|37676]] and duplicated in at least [[https://bugs.php.net/bug.php?id=39915|39915]] and [[https://bugs.php.net/bug.php?id=72636|72636]]. PHP correctly returns a warning when you attempt to use an offset that is of an invalid type, PHP should also provide a warning when a container is of an invalid type. |
| |
| ===== Proposal ===== | ===== Proposal ===== |
| This RFC proposed to combine a couple of existing PR's[[https://wiki.php.net/rfc/notice-for-non-valid-array-container#references|[1]]][[https://wiki.php.net/rfc/notice-for-non-valid-array-container#references|[2]]] as well as my implementation to raise a E_NOTICE when a container type is used that does not contain array-accessable data. The default behavior, of a silent NULL return, is not correctly defined in the [[http://php.net/manual/en/language.types.array.php|Array Documentation]] however is noted in a [[http://php.net/manual/en/language.types.array.php#111367|comment]] | This RFC proposed to combine a couple of existing PR's[[https://wiki.php.net/rfc/notice-for-non-valid-array-container#references|[1]]][[https://wiki.php.net/rfc/notice-for-non-valid-array-container#references|[2]]] as well as my implementation to raise a E_WARNING when a container type is used that does not contain array-accessable data. The default behavior, of a silent NULL return, is not correctly defined in the [[http://php.net/manual/en/language.types.array.php|Array Documentation]] however is noted in a [[http://php.net/manual/en/language.types.array.php#111367|comment]] |
| |
| As compared to the previous PR's (referenced below) this proposed implementation takes effort to limit throwing errors multiple times. | As compared to the previous PR's (referenced below) this proposed implementation takes effort to limit throwing errors multiple times. |
| <?php | <?php |
| $var = false; | $var = false; |
| $var[1][2][3][4][5]; // This will throw a single E_NOTICE, as accessing ($a[1])[2] | $var[1][2][3][4][5]; // This will throw a single E_WARNING, as accessing ($a[1])[2] |
| // is attempting to access a IS_VAR chain that is NULL. | // is attempting to access a IS_VAR chain that is NULL. |
| // Output would be: | // Output would be: |
| // Notice: Variable of type boolean does not accept array offsets | // Warning: Variable of type boolean does not accept array offsets |
| | |
| $var = array(123); | $var = array(123); |
| $var[0][1]; // This will throw an E_NOTICE, as accessing $a[0] is valid | $var[0][1]; // This will throw an E_WARNING, as accessing $a[0] is valid |
| // [1] is accessing an integer as an array. | // [1] is accessing an integer as an array. |
| // Output would be: | // Output would be: |
| // Notice: Variable of type integer does not accept array offsets | // Warning: Variable of type integer does not accept array offsets |
| | |
| | // Brought up during vote: |
| | $a = [null]; |
| | $c = null; |
| | var_dump($a[0][0] + $c[0]); // This will through 2 E_WARNINGS, First would be $a[0][0] |
| | // For the same reason as above, $a[0] is rightfully NULL |
| | // and accessing [0] on null is invalid. The second, |
| | // would be $c[0] for the same reason. |
| | // Output: |
| | // int(0) |
| </file> | </file> |
| |
| ===== Backward Incompatible Changes ===== | ===== Backward Incompatible Changes ===== |
| In terms of error-reporting for default INI settings there would be no BC-break for production, but there would be notices for development. Many production deployments alter default INI settings for error-reporting and as such could be exposed to this notice displaying. This would need to be documented in release notes for users to properly address in development environments. | In terms of error-reporting for default INI settings there would be no BC-break for production, but there would be warnings for development. Many production deployments alter default INI settings for error-reporting and as such could be exposed to this warning displaying. This would need to be documented in release notes for users to properly address in development environments. |
| | |
| | However, error-reporting isn't the only aspect that needs to be addressed here. ''set_error_handler()'', if listening to E_WARNING will be triggered for this warning regardless of INI settings. As well ''error_get_last()'' would also contain this warning if it was the most recent error. These two operate regardless of ''error_reporting(0)''. |
| | |
| | This RFC aims to limit the quantity of warnings on a single line, however, large projects may have many locations that might need variable type checking around unknown container access. Typically, one might assume access of a variable to be an array or object, and could do checks to make sure the variable about to be accessed is an array, or an object. |
| | |
| | ===== Performance Impact ===== |
| | Test run was: |
| | <file php> |
| | <?php |
| | $a = false; |
| | for ($i = 0; $i < 1000000; $i++) { |
| | $a[0]; |
| | } |
| | </file> |
| |
| However, error-reporting isn't the only aspect that needs to be addressed here. ''set_error_handler()'', if listening to E_NOTICE will be triggered for this notice regardless of INI settings. As well ''error_get_last()'' would also contain this notice if it was the most recent error. These two operate regardless of ''error_reporting(0)''. | Execution Time (DualCore 3ghz; 2g ram) |
| | * Current Master : ~0.09s (~489m operations) |
| | * Current Master w/RFC Displaying Warnings: ~33.25s (~7.799b operations) |
| | * Current Master w/RFC Hiding Warnings: ~0.82s (~4.091b operations) |
| |
| This RFC aims to limit the quantity of notices on a single line, however, large projects may have many locations that might need variable type checking around unknown container access. Typically, one might assume access of a variable to be an array or object, and could do checks to make sure the variable about to be accessed is an array, or an object. | We can see there is a significant increase in operations on huge loads. However, I wouldn't suspect 1m of these errors per request almost ever. So, yes there is; but I'd call it useful information overhead. |
| |
| ===== Proposed PHP Version(s) ===== | ===== Proposed PHP Version(s) ===== |
| No Opcodes are touched with this RFC | No Opcodes are touched with this RFC |
| |
| ===== Open Issues ===== | ==== To Documentation ==== |
| | It would probably be nice to document the E_WARNING behavior for accessing on non-array types in the Array Documentation either [[http://php.net/manual/en/language.types.array.php#language.types.array.syntax.accessing|here]] or [[http://php.net/manual/en/language.types.array.php#language.types.array.donts|here]] |
| | |
| | ===== Discussed Issues ===== |
| | ==== E_NOTICE or E_WARNING ==== |
| | I began this RFC with the implementation raising an E_NOTICE. However, when attempting to access a scalar value as array for a [[https://github.com/bp1222/php-src/blob/7369cfcc0f215156eafae71e2b62a573512b3d05/Zend/zend_execute.c#L1776|write-context]] it raises an E_WARNING. So I'm mimicking this behavior when accessing any scalar not just those <= IS_FALSE on a read-context. |
| ==== NULL Identity ==== | ==== NULL Identity ==== |
| Should NULL variables be treated as a special identity so that accessing array on a null just returns null. | Should NULL variables be treated as a special identity so that accessing array on a null just returns null. |
| * ''foreach(list($a, $b) = each($foo))'' -- Would need to prevent warn on 'final' null each | * ''foreach(list($a, $b) = each($foo))'' -- Would need to prevent warn on 'final' null each |
| |
| Per discussion on the [[https://github.com/php/php-src/pull/2031#issuecomment-238366706|PR]] I have limited this RFC to not raise notices when setting any value by use of list(). | Per discussion on the [[https://github.com/php/php-src/pull/2031#issuecomment-238366706|PR]] I have limited this RFC to not raise warnings when setting any value by use of list(). |
| |
| ==== Reference Assignment Access ==== | ==== Reference Assignment Access ==== |
| $value = null; | $value = null; |
| $dim = 0; | $dim = 0; |
| $new_val = $null[$dim]; | $new_val =& $value[$dim]; |
| var_dump($null); | var_dump($value); |
| /* | /* |
| Output: | Output: |
| </file> | </file> |
| |
| In the above example the variable ''$null'' is accessed via write, and current operations there instruct the variable to become an array, where $dim is null, and then create that as a reference. I dislike how this is unique to NULL values being accessed, wherein bools/floats/ints silently fail, and the return value is undefined. Regardless, because this is accessed as a write, it falls outside the scope of this RFC which aims to raise a notice for read access. | In the above example the variable ''$null'' is accessed via write, and current operations there instruct the variable to become an array, where $value[$dim] is created as null, and then make it a reference. I dislike how this is unique to NULL values being accessed, wherein bools/floats/ints silently fail, and the return value is undefined. Regardless, because this is accessed as a write, it falls outside the scope of this RFC which aims to raise a warning for read access. |
| |
| ===== Proposed Voting Choices ===== | ===== Proposed Voting Choices ===== |
| No syntax is changed, a vote of 50%+1 will be necessary | Requires 2/3 Vote |
| | <doodle title="E_WARNING for invalid container read array-access" auth="bp1222" voteType="single" closed="false"> |
| | * Yes |
| | * No |
| | </doodle> |
| | Vote Start: 2016-08-16 15:36 |
| | |
| | Vote End: 2016-08-31 23:59 |
| |
| ===== Patches and Tests ===== | ===== Patches and Tests ===== |
| [2] [[https://github.com/php/php-src/pull/1269|PR 1269]] | [2] [[https://github.com/php/php-src/pull/1269|PR 1269]] |
| |
| ==== Previous Discussion ==== | ==== Initial Email to Internals ==== |
| [3][[http://marc.info/?t=143379796900001&r=1&w=2|Email Thread]] | [[http://marc.info/?l=php-internals&m=146999353828790&w=2|Email Thread]] |
| | |
| | ==== Previous Email Discussion ==== |
| | [[http://marc.info/?t=143379796900001&r=1&w=2|Email Thread]] |