Replace file based extension tests with inline ones #180
Conversation
crazywhalecc
added
need test
|
I feel that including code in json is not a good solution. Maybe directly reading the existing xxx.php and use |
|
I may agree that it looks confusing to have php-alike code in json file. I will play with other options today. In the worst case, are you open to replacing that json file with another approach? (like php files or array?) |
|
I have previously considered using PHP to replace the current JSON format configuration file, for example Now in the end I decided to use json and completely separate the PHP code for a simple purpose: everything seems to be low coupling, these configuration files are more like metadata that does not change, and the compiled code and parameters are like functions part. If we use PHP as a configuration file, it is inevitable that dynamically executed code will be included in the configuration file. Maybe writing all configuration items directly as classes and deleting the configuration files is a more reasonable choice (just like the solution I originally abandoned). |
|
Now it is indeed a troublesome question: should the test code of these extensions be part of the logic code or a static configuration? Honestly I have no idea. This is why I planned to put these test files into a separate tests directory. |
|
I vote for separation of test code from configuration metadata. The tests MUST be PHP (if you're using PHP to run them), the other can be anything (e.g. JSON) Why not simply include a relative path reference in the JSON metadata to any test file? e.g. "bcmath": {
"type": "builtin"
"type": "builtin",
"tests": "./tests/bcmath.php"
},This is explicit, can easily be resolved from whichever context is executing the build process, and acts as a pointer for any developer looking to see how things are wired up without having to dive into code. |
|
@simonhamp This seems to be possible and a compromise. Anyway, I think we finally use |
|
@crazywhalecc the I guess I'm struggling to see how changing to using It feels super brittle to be interpolating PHP from a file (any file, JSON or PHP) into a shell string. |
@simonhamp This is true. But in general, these test codes are theoretically controllable, because they are only in the src directory within the project, and everyone can check their security. What may need to be done is to escape special characters. I also mentioned three options I can think of in issue #124, the first two seem riskier than this one. In addition, I also thought that on Unix platform, it can be executed through pipes:
But this solution also has problems: it is not possible to obtain its own executable path now after packaging Phar and phpmicro executable. |
|
@crazywhalecc I agree with you, there's minimal concern from a security perspective. I meant 'brittle' in the sense that they could be the source of various gotchas with interpolation depending on how convoluted the test needs to be, something that even escaping may not help with. It still feels like a path resolution issue to me... |
I have convinced myself not to trapped in this Phar path issue, because after using phar and phpmicro sfx, everything related to the path became anxious, whether reading an internal file from inside Phar or outside. I once thought that the ULTIMATE is to manually generate a PHP file containing the test code for each extension, and then call it on demand: |
|
For me the best for now (till someone finds "better" way) is to inline the test files with trim as suggested by @crazywhalecc, provided an alternative PR: #182 Closing this one as no-go. |
This change simplifies testing of extensions against custom build PHP binaries, also inside the phar!
stloyd@MacBook-Pro-2 spc-test % ./spc build curl,dom --build-cli _ _ _ _ ___| |_ __ _| |_(_) ___ _ __ | |__ _ __ / __| __/ _` | __| |/ __|____| '_ \| '_ \| '_ \ \__ \ || (_| | |_| | (_|_____| |_) | | | | |_) | |___/\__\__,_|\__|_|\___| | .__/|_| |_| .__/ v2.0-rc6 |_| |_| [17:28:32] [INFO] [EXEC] sysctl -n hw.ncpu [17:28:32] [INFO] Build target: cli [17:28:32] [INFO] Enabled extensions: curl, dom [17:28:32] [INFO] Required libraries: zlib, openssl, curl, libiconv, libxml2 (...) [17:31:01] [INFO] [EXEC] make -j10 EXTRA_CFLAGS='-g -Os' EXTRA_LIBS=' -liconv -framework CoreFoundation -framework SystemConfiguration /Users/stloyd/Documents/spc-test/buildroot/lib/libxml2.a /Users/stloyd/Documents/spc-test/buildroot/lib/libiconv.a /Users/stloyd/Documents/spc-test/buildroot/lib/libcharset.a /Users/stloyd/Documents/spc-test/buildroot/lib/libcurl.a /Users/stloyd/Documents/spc-test/buildroot/lib/libssl.a /Users/stloyd/Documents/spc-test/buildroot/lib/libcrypto.a /Users/stloyd/Documents/spc-test/buildroot/lib/libz.a -lresolv' cli [17:33:29] [INFO] [EXEC] dsymutil -f sapi/cli/php [17:33:30] [INFO] [EXEC] strip sapi/cli/php [17:33:30] [INFO] Deploying cli file [17:33:30] [INFO] [EXEC] cp '/Users/stloyd/Documents/spc-test/source/php-src/sapi/cli/php' '/Users/stloyd/Documents/spc-test/buildroot/bin/' [17:33:30] [INFO] running cli sanity check [17:33:30] [INFO] [EXEC] /Users/stloyd/Documents/spc-test/buildroot/bin/php -r "echo \"hello\";" [17:33:31] [INFO] [EXEC] /Users/stloyd/Documents/spc-test/buildroot/bin/php -r "assert(function_exists('curl_init'));" [17:33:32] [INFO] [EXEC] /Users/stloyd/Documents/spc-test/buildroot/bin/php -r "assert(class_exists('\DOMDocument'));$doc = new DOMDocument();$doc->loadHtml('<html><head><meta charset="UTF-8"><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"></head><body id='app'>Hello</body></html>');assert($doc->getElementById('app')->nodeValue === 'Hello');" [17:33:32] [INFO] Build complete, used 299.775 s ! [17:33:32] [INFO] Static php binary path: /Users/stloyd/Documents/spc-test/buildroot/bin/php [17:33:32] [INFO] License path: /Users/stloyd/Documents/spc-test/buildroot/license/