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/24 15:48] – 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 |
</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_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)''. | 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 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. | 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> |
| |
| 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) |
| |
| 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]] |