Skip to content

Fixes bug 75928 #3081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Fixes bug 75928 #3081

wants to merge 2 commits into from

Conversation

pslacerda
Copy link

@pslacerda pslacerda commented Feb 7, 2018

This fixes DateTimeZone::listIdentifiers ensuring that null is a valid parameter by calling Z_PARAM_STRING_EX directly.

@carusogabriel
Copy link
Contributor

@pslacerda Maybe we can modify some tests, like its alias timezone_identifiers_list, to ensure everything is right? Some tests:

@nikic
Copy link
Member

nikic commented Feb 7, 2018

@carusogabriel Modifying an existing test is problematic because this requires strict_types=1, but I agree that it would be good to add a (new) test for this change.

@krakjoe
Copy link
Member

krakjoe commented Feb 8, 2018

It would be ideal if this change came with appropriate test(s)

@pslacerda
Copy link
Author

I agree!

Until next monday I'll make some tests. Searching on github I found no usages of Z_PARAM_STRING_EX, so maybe look for usage candidates on documentation may be a good starting point to find other similar errors, if they exist. What do you think?

By the way function signatures are very neat on PHP source code, even if usage failure of macros occurs eventually.

@pslacerda
Copy link
Author

pslacerda commented Feb 10, 2018

Tests done, was basically a copy of the one in the bug description. :-)

I'm searching by the files refs.basic.php.php, refs.utilspec.audio.php etc to scan the documentation for eventual similar errors. Can you show me where they are?

My starting point was the manual in the web-php repo. The strategy is to look for strings with null as default.

https://github.com/php/web-php/blob/master/manual/en/toc/funcref.inc

By the way there are many already done testes in the documentation, for instance the Example 1 of apc_add. They are already made?

http://php.net/manual/en/function.apc-add.php

If they are already made I guess that we can run all that with and without strict_types=1 and the results must be the same, correct? This is another cool task. Or I'm missing something?

This documentation testing will not always work, because examples like scandir. Others seems to be fragmented into to examples like SimpleXMLElement. But I'm pretty sure that it will work many times.

http://php.net/manual/en/function.scandir.php
http://php.net/manual/en/simplexml.examples-basic.php

@nikic
Copy link
Member

nikic commented Feb 10, 2018

Merged as fddd7e3 into 7.1+ with some simplification to the test file.

I can't answer your docs related questions, not really familiar with the infrastructure there. The docs are located at http://svn.php.net/viewvc/phpdoc/ (though edit.php.net might be more convenient for browsing), but I don't know if there's an easy way to extract the example codes. I think it might be more fruitful to run phpt tests with strict_types -- however, there are also many many tests that explicitly test invalid inputs, so the signal to noise ratio will probably not be great.

@pslacerda
Copy link
Author

pslacerda commented Feb 11, 2018

Very nice, my first PHP contribution! And thank you for simplify the test. I'm doing my best to learn PHP internals and maybe contribute back and currently I'm very interested in testing in general.

I adapted the run-tests.php to accept a -t flag that inserts declare(strict_types=1); in the --FILE-- section. It really happened what you said, many tests for invalid inputs failed.

However seems that most if not all tests of the few ones in ext with strict_types are related to bugs.

  • sodium/tests/sodium_error_001.phpt
  • date/tests/timezones-list-strict.phpt
  • mysqli/tests/bug74547.phpt
  • zlib/tests/zlib_wrapper_level.phpt
  • gettext/tests/bug73730.phpt

This indicates that tests with strict_types are somehwat undertested, but a manual validation at function preamble must be made to strict typing be correctly handled (ex ZEND_ARG_USES_STRICT_TYPES) so they may also need specific tests for invalid input.

I would say to put a --SKIPIF-- section checking if it is enabled but I found no way to do the equivalent of CG(active_op_array)->fn_flags & ZEND_ACC_STRICT_TYPES in a pure .php file.

If I want to edit tests to skipif in that case and run all the suite with and without -t, what should I do?

@pslacerda
Copy link
Author

About the docs, that repo is what I needed. Maybe some unofficial tests can be made with that.

@pslacerda
Copy link
Author

pslacerda commented Feb 11, 2018

I just made some modifications enabling testing with strict_types. A new function check_strict_types() was created enabling the PHP test to skip if true.

Edit: fixed https://github.com/php/php-src/compare/master...pslacerda:experimental/strict_testing?diff=split

$ ./run-tests.php -t ext/date/tests/timezone_transitions_get_variation3.phpt

I imagine that such function check_strict_types() didn't existed before because had no valid use cases and maybe also avoid the user to trick the interpreter. But seems really useful to run all possible tests with strict_types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants