PHP 8.4 #541

mykiwi · 2025-01-03T18:18:00Z

PHP 8.4

TLDR: 179/180 (+2 skip) tests passed (99.4%)

php 8.4.2 (on x86_64) test report:

PHP 8.3

TLDR: 179/180 (+2 skip) tests passed (99.4%)

php 8.3.15 (on x86_64) test report:

PHP 8.2

TLDR: 179/180 (+2 skip) tests passed (99.4%)

php 8.2.27 (on x86_64) test report:

PHP 8.1

TLDR: 179/180 (+2 skip) tests passed (99.4%)

php 8.1.31 (on x86_64) test report:

=====================================================================
TIME END 2025-01-09 09:12:46

=====================================================================
TEST RESULT SUMMARY
---------------------------------------------------------------------
Exts skipped    :    0
Exts tested     :   33
---------------------------------------------------------------------

Number of tests :  182               180
Tests skipped   :    2 (  1.1%) --------
Tests warned    :    0 (  0.0%) (  0.0%)
Tests failed    :    1 (  0.5%) (  0.6%)
Tests passed    :  179 ( 98.4%) ( 99.4%)
---------------------------------------------------------------------
Time taken      :    9 seconds
=====================================================================

=====================================================================
FAILED TEST SUMMARY
---------------------------------------------------------------------
Test V8::executeString() : Time limit [tests/time_limit.phpt]
=====================================================================

bastien-wink · 2025-01-04T17:10:46Z

Interesting test run with docker and bake.
Do you consider also updating .github/workflows/build-test.yml ?

Most important is to fix those 11 tests so we can use PHP 8.4.

JoshuaBehrens · 2025-01-04T17:35:18Z

@mykiwi there does not seem to be a reason for the version check. In #539 I was able to solve it without php version check

Why do we need a dockerfile here?

JoshuaBehrens · 2025-01-04T17:39:25Z

Time limit is also failing for me on MacOS with php8.3. Unsure right now how to solve it though. There are also issue right now for me as some things are missing around the DateTime timezone name lookup :/

stesie · 2025-01-04T18:45:20Z

I'd also say ignore the time limit ones for now. Locally on my Mac they fail as well. On the Linux machine (and CI) they pass just fine. TZ one likewise.

The others seem "just" to be caused by different stack traces (at least those I've checked)

If you compare

root@e48d4f358c52:/tmp/php-v8js# diff -u tests/commonjs_exception_001.{exp,out}
--- tests/commonjs_exception_001.exp	2025-01-04 18:36:44.627206918 +0000
+++ tests/commonjs_exception_001.out	2025-01-04 18:36:44.628206860 +0000
@@ -1,15 +1,15 @@
-Fatal error: Uncaught Exception: some exception in %s%ecommonjs_exception_001.php:9
+Fatal error: Uncaught Exception: some exception in /tmp/php-v8js/tests/commonjs_exception_001.php:9
 Stack trace:
-#0 [internal function]: {closure}('test')
-#1 %s%ecommonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
+#0 [internal function]: {closure:/tmp/php-v8js/tests/commonjs_exception_001.php:8}('test')
+#1 /tmp/php-v8js/tests/commonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
 #2 {main}
 
-Next V8JsScriptException: module.js:1: Exception: some exception in %s%ecommonjs_exception_001.php:9
+Next V8JsScriptException: module.js:1: Exception: some exception in /tmp/php-v8js/tests/commonjs_exception_001.php:9
 Stack trace:
-#0 [internal function]: {closure}('test')
-#1 %s%ecommonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
-#2 {main} in %s%ecommonjs_exception_001.php:12
+#0 [internal function]: {closure:/tmp/php-v8js/tests/commonjs_exception_001.php:8}('test')
+#1 /tmp/php-v8js/tests/commonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
+#2 {main} in /tmp/php-v8js/tests/commonjs_exception_001.php:12
 Stack trace:
-#0 %s%ecommonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
+#0 /tmp/php-v8js/tests/commonjs_exception_001.php(12): V8Js->executeString('var foo = requi...', 'module.js', 4)
 #1 {main}
-  thrown in %s%ecommonjs_exception_001.php on line 12
\ No newline at end of file
+  thrown in /tmp/php-v8js/tests/commonjs_exception_001.php on line 12
\ No newline at end of file

The expectation has just {closure} while the actual output on php 8.4 is {closure:/tmp/php-v8js/tests/commonjs_exception_001.php:8}
... therefore if you replace the expectation by {closure%s}, then it should match both, php 8.4 and the older ones

root@e48d4f358c52:/tmp/php-v8js# diff -u tests/generators_from_v8_008.{exp,out}
--- tests/generators_from_v8_008.exp	2025-01-04 18:36:46.819080332 +0000
+++ tests/generators_from_v8_008.out	2025-01-04 18:36:46.819080332 +0000
@@ -1,9 +1,9 @@
 int(23)
 
-Fatal error: Uncaught Exception: this shall not work in %s
+Fatal error: Uncaught Exception: this shall not work in /tmp/php-v8js/tests/generators_from_v8_008.php:14
 Stack trace:
-#0 [internal function]: {closure}()
+#0 [internal function]: {closure:/tmp/php-v8js/tests/generators_from_v8_008.php:13}()
 #1 [internal function]: Closure->__invoke()
-#2 %s: V8Generator->next()
+#2 /tmp/php-v8js/tests/generators_from_v8_008.php(18): V8Generator->next()
 #3 {main}
-  thrown in %s
\ No newline at end of file
+  thrown in /tmp/php-v8js/tests/generators_from_v8_008.php on line 14
\ No newline at end of file

... here it now also says {closure:/tmp/php-v8js/tests/generators_from_v8_008.php:13}

stesie · 2025-01-04T18:48:21Z

and maybe one more thought: I think it might be confusing to users, that a Dockerfile shipped with a project doesn't yield a useable binary, but just test results.

What do you think of naming it e.g. Dockerfile.test or so ?

PS: is there some neat way to run the buildx bake stuff with the local repo? Am I missing something obvious?

mykiwi · 2025-01-05T11:05:16Z

@bastien-wink : Do you consider also updating .github/workflows/build-test.yml ?

If maintainers want it, I can.

@bastien-wink : Most important is to fix those 11 tests so we can use PHP 8.4.

Yes, that where I needed help 🙌

@JoshuaBehrens : Why do we need a dockerfile here?

For me it is to be able to test it easily on your machine, with all benefits of docker: easy to tests with many php versions/distros/platforms, having cache, reproducibility.

@stesie : is there some neat way to run the buildx bake stuff with the local repo?

Sure, like this: docker buildx bake --set main.contexts.php-v8js-source-code=.
I forgot to push my Makefile with all thoses commands.

@stesie I think it might be confusing to users, that a Dockerfile shipped with a project doesn't yield a useable binary, but just test results. What do you think of naming it e.g. Dockerfile.test or so ?

What about creating a folder name devtools (or something else than "docker" to avoid confusion) and put files like the docker-bake.hcl, foobar.Dockerfile, ... ?

JoshuaBehrens · 2025-01-06T05:13:27Z

I 👍 a devtools folder. I am on devenv to switch easily between php versions (I test with brew though as it is common macOS installation way). I also 👍 the Dockerfile as dev utility as this would be a predefined linux environment to test for on any dev machine.

For me it is to be able to test it easily on your machine, with all benefits of docker: easy to tests with many php versions/distros/platforms, having cache, reproducibility

I see the default docker pitch but for this scenario I see it as a big 👍 . I see downsides but this would not be applicable just for the sake of it as a devtool 😁

The makefile looks pretty standard with the default help and clean target and should be therefore picked up easily by people who are used to work with make. Likely I would not move it into the devtools folder though as it would be the command set to control the devtools. So everything in the devtools folder and the makefile as the single entrypoint in root is plausible to me.

If maintainers want it, I can

I would be happy if it is updated as this is the part which makes sure, that all "common" scenarios should be tested. Building v8 though takes a lot of time and therefore one really needs to make sure, that the steps, that compile v8 themselves are cached properly.

Eventually I don't want to express a leading opinion here though ^^' @stesie is the one to decide.

stesie · 2025-01-06T12:36:54Z

Eventually I don't want to express a leading opinion here though ^^' @stesie is the one to decide.

mmmh, after all it has to meet all our requirements, and I don't want this to be up to my taste only.

Generally I like the idea of having a subfolder for all that fluff. And I agree that the Makefile should be top-level so it's easily available.

Regarding the CI stuff, if it easily fits there, why not re-use it. Much of it is different anyways ...
On the other hand, feel free to keep it off this PR to keep it small and then maybe follow-up with another one.

Most important is to fix those 11 tests so we can use PHP 8.4.
Yes, that where I need help 🙌

mmmh, I'm a bit confused ... do you still need help or are you good to go? Have you tried changing the {closure} expectation in the .phpt files to {closure%s}? Did it work? further errors?

TBH I haven't tried. So feel free to ask for more support, then I'll have a deeper look and try it myself. But I'm pretty sure that's the only issue you've got

mykiwi · 2025-01-06T13:03:38Z

do you still need help or are you good to go? Have you tried changing the {closure} expectation in the .phpt files to {closure%s}? Did it work? further errors?

I don't think, thanks! Not yet, but I will try.

mykiwi · 2025-01-07T00:04:38Z

For information, I did {closur%s} instead of {closure%s} to keep tests pass on previous versions

I have 2 last errors:

Test V8::executeString() : Time limit [tests/time_limit.phpt]
Test V8::executeString() : var_dump [tests/var_dump.phpt]

If you have any idea on how to fix them, feel free to guide me

stesie · 2025-01-08T23:23:10Z

First of all, and out of interest, why do we mix PHP 8.4 with changes in tooling here? TBH I think discussions would be way cleaner, if we'd sort out that stuff separately.

That said, I'm still not totally sold to the new build stuff, what's the advantage of it, etc. ... but maybe that's also just because I'm doing it manually/differently for ten years now :) ... so I'm totally up to adding this, but to me it would be helpful if there would be some guiding words (read: documentation), which problem it solves, how it's suppose to be used, and importantly how to debug it.

So yes, if I'm running it, I get the final result in tests-outputs/php8.4.2_x86_64__1-std (took me a while to find it), but that's only the console output. How am I supposed to access the exp/out files of the failed tests? To access these I have to somehow copy that stuff from the docker images, right ?

what's worse, this now adds a Makefile in toplevel dir (yes, sorry, I even asked for that, but have overseen stuff) ... however, if you run the ./configure script, then it overrides that Makefile. And since it's under version control then wants to commit that.
Which is bad IMHO ... so maybe move that file to devtools as well, and then document that recommended approach is to run make -f devtools/Makefile php84 or so

regarding php 8.4 changes

with the new docker build both tests, you've mentioned, fail for me as well.

If I build it the "classic" way (on Ubuntu 24.04, amd64 machine) with ppa:ondrej/php active and php8.4-dev/cli installed from there

./configure --with-v8js=/opt/v8-13.1.63 LDFLAGS=-lstdc++ 'CPPFLAGS=-DV8_COMPRESS_POINTERS  -DV8_ENABLE_SANDBOX -Wno-invalid-offsetof -Wno-deprecated-declarations -Wno-unused-result' --with-php-config=/usr/bin/php-config8.4
make clean
make -j4
make test

The time_limit one passes just fine ... that one is "known" to be flaky ... I've seen it failing on MacOS as well. So I wouldn't care about that one.

regarding the var_dump.phpt ... if you have a look at the .out file

the first var_dump is producing the following output for the "function" key

  ["function"]=>
  object(Closure)#4 (4) {
    ["name"]=>
    string(58) "{closure:/home/stesie/Projekte/v8js/tests/var_dump.php:51}"
    ["file"]=>
    string(45) "/home/stesie/Projekte/v8js/tests/var_dump.php"
    ["line"]=> 
    int(51)
    ["parameter"]=>
    array(1) {
      ["$x"]=>
      string(10) "<required>"
    }
  }

however the expectation looks like this

  \["function"\] \=\>
  object\(Closure\)\#\d+ \{
      function id\(x\) \{ return x; \}
  \}

... or to put it differently, php 8.4 completely changed the output of var_dump here :)

Also for example the DateTime object, which is also printed, now has more methods.

I think having dedicated tests, one testing the closure/datetime printing on php < 8.4 and one for php >= 8.4 would be better, even though it would be a bit more redundant.

And while being at that test file, you also might want to consider (partialy) reverting my commit

commit fcd78f830ac29fdc4e48834e313383e3d15665e1
Author: Stefan Siegl <[email protected]>
Date:   Sun Jun 23 19:09:12 2019 +0200

    use EXPECTREGEX to handle different PHP version's output well

... which introduced that EXPECTREGEX usage, which is really hard to maintain.

Or to put it differently, I'd suggest splitting the var_dump.phpt test into a var_dump_php83_and_older.phpt and a var_dump_php84_and_newer.phpt ... that share the same implementation, but have (slightly) different test expectations (and you use the SKIPIF blocks to skip either this or that; see above commit on how that can be done)

…k) and fix some tests Signed-off-by: Romain Gautier <[email protected]>

Signed-off-by: Romain Gautier <[email protected]>

mykiwi · 2025-01-09T09:20:33Z

@stesie First of all, and out of interest, why do we mix PHP 8.4 with changes in tooling here? TBH I think discussions would be way cleaner, if we'd sort out that stuff separately.

No problem. I updated the PR to only contains what is useful for php8.4

So yes, if I'm running it, I get the final result in tests-outputs/php8.4.2_x86_64__1-std (took me a while to find it), but that's only the console output.

If you don't have tests-outputs/php8.4.2_x86_64__2-report, that means something wrong append and maybe there are some details why in tests-outputs/php8.4.2_x86_64__1-std

I'd suggest splitting the var_dump.phpt test into a var_dump_php83_and_older.phpt and a var_dump_php84_and_newer.phpt ... that share the same implementation, but have (slightly) different test expectations (and you use the SKIPIF blocks to skip either this or that; see above commit on how that can be done)

Done

stesie · 2025-01-09T19:24:29Z

@mykiwi thank you very much for your contribution!

I've pushed one more minor fix, the test fixture was asserting for the build location, which obviously isn't ideal :)

diff --git a/tests/var_dump.phpt b/tests/var_dump.phpt
index 9097ca9..13ca64e 100644
--- a/tests/var_dump.phpt
+++ b/tests/var_dump.phpt
@@ -114,9 +114,9 @@ array(11) {
   ["function"]=>
   object(Closure)#4 (4) {
     ["name"]=>
-    string(45) "{closure:/tmp/php-v8js/tests/var_dump.php:51}"
+    string(%d) "{closure:%s/tests/var_dump.php:51}"
     ["file"]=>
-    string(32) "/tmp/php-v8js/tests/var_dump.php"
+    string(%d) "%s/tests/var_dump.php"
     ["line"]=>
     int(51)
     ["parameter"]=>

apart from that, nice work, thanks :)

mykiwi force-pushed the php84-and-tooling branch from 0719d54 to 86f373e Compare January 3, 2025 18:38

mykiwi force-pushed the php84-and-tooling branch 3 times, most recently from d5c2a24 to f697ed2 Compare January 7, 2025 09:42

mykiwi marked this pull request as ready for review January 7, 2025 09:47

mykiwi added 2 commits January 9, 2025 09:36

adapt code to handle php 8.4 too (based on @JoshuaBehrens & @rbro wor…

6386e9d

…k) and fix some tests Signed-off-by: Romain Gautier <[email protected]>

update var_dump test to handle php84 and previous versions

c2614b5

Signed-off-by: Romain Gautier <[email protected]>

mykiwi force-pushed the php84-and-tooling branch from d3c99d3 to c2614b5 Compare January 9, 2025 09:11

mykiwi changed the title ~~PHP 8.4 (and docker tooling/documentation)~~ PHP 8.4 Jan 9, 2025

JoshuaBehrens approved these changes Jan 9, 2025

View reviewed changes

stesie added a commit that referenced this pull request Jan 9, 2025

Fix tests/var_dump.phpt to not require certain build path, refs #541

38c0d6b

stesie merged commit c2614b5 into phpv8:php8 Jan 9, 2025
2 of 3 checks passed

stesie mentioned this pull request Jan 9, 2025

php 8.4 compile error - php_strtolower not declared #540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHP 8.4 #541

PHP 8.4 #541

mykiwi commented Jan 3, 2025 •

edited

Loading

bastien-wink commented Jan 4, 2025

JoshuaBehrens commented Jan 4, 2025 •

edited

Loading

JoshuaBehrens commented Jan 4, 2025

stesie commented Jan 4, 2025

stesie commented Jan 4, 2025

mykiwi commented Jan 5, 2025 •

edited

Loading

JoshuaBehrens commented Jan 6, 2025

stesie commented Jan 6, 2025

mykiwi commented Jan 6, 2025

mykiwi commented Jan 7, 2025 •

edited

Loading

stesie commented Jan 8, 2025

mykiwi commented Jan 9, 2025 •

edited

Loading

stesie commented Jan 9, 2025

PHP 8.4 #541

PHP 8.4 #541

Conversation

mykiwi commented Jan 3, 2025 • edited Loading

PHP 8.4

PHP 8.3

PHP 8.2

PHP 8.1

bastien-wink commented Jan 4, 2025

JoshuaBehrens commented Jan 4, 2025 • edited Loading

JoshuaBehrens commented Jan 4, 2025

stesie commented Jan 4, 2025

stesie commented Jan 4, 2025

mykiwi commented Jan 5, 2025 • edited Loading

JoshuaBehrens commented Jan 6, 2025

stesie commented Jan 6, 2025

mykiwi commented Jan 6, 2025

mykiwi commented Jan 7, 2025 • edited Loading

stesie commented Jan 8, 2025

mykiwi commented Jan 9, 2025 • edited Loading

stesie commented Jan 9, 2025

mykiwi commented Jan 3, 2025 •

edited

Loading

JoshuaBehrens commented Jan 4, 2025 •

edited

Loading

mykiwi commented Jan 5, 2025 •

edited

Loading

mykiwi commented Jan 7, 2025 •

edited

Loading

mykiwi commented Jan 9, 2025 •

edited

Loading