Deprecate disabling use_only_cookies

[kamil-tekiela]

No description provided.

[kamil-tekiela]

I don't know why the Windows tests are failing. Can someone assist?

[nielsdos]

I don't know why the Windows tests are failing. Can someone assist?

Add an INI section to the tests where you set error_reporting=-1, the default Windows configuration used in CI doesn't set E_DEPRECATED by default in its error_reporting. That should make them pass.
In any case, setting the error_reporting explicitly in the INI section also avoids false failures when people run the tests with their own php.ini.

[nielsdos]

Looks like it'll still fail because they come from startup, i.e. before it had the chance of updating the INI options. Probably the CI runner configuration should then be adjusted instead...

[kamil-tekiela]

Where do I change the CI configuration?

[nielsdos]

If I had to take a guess, the error reporting is set too low in the php.ini file used for the Windows CI, but I'm not sure without digging into it where that's set. @iluuu1994 did set up the CI so he might know.

[iluuu1994]

Windows apparently only does release builds, which we should probably change. You could try changing this line:

php-src/.github/scripts/windows/build_task.bat

Line 43 in d9a9696

--disable-debug-pack ^

[Girgias]

Looks mostly good just some minor comments that need addressing.

[kamil-tekiela]

Windows apparently only does release builds, which we should probably change. You could try changing this line:

php-src/.github/scripts/windows/build_task.bat

Line 43 in d9a9696

--disable-debug-pack ^

This did not help. See the last commit.

[iluuu1994]

Ok, I don't know then. error_reporting is handled by run-tests.php, so there's shouldn't be a need to set it again.

[jorgsowa]

RFC for reference: https://wiki.php.net/rfc/deprecate-get-post-sessions

[kocsismate]

@cmb69 I cc you in the hope that you how to fix the Windows failure :)

[cmb69]

When running nmake test TESTS=ext\session\tests\rfc1867_inter.phpt, the relevant system_with_timeout() of run-tests.php returns:

Deprecated: PHP Startup: Disabling session.use_only_cookies INI setting is deprecated in Unknown on line 0
X-Powered-By: PHP/8.4.0-dev
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate
Pragma: no-cache
Content-type: text/html; charset=UTF-8

string(13) "rfc1867-inter"
array(2) {
  ["file1"]=>
  array(6) {
<rest omitted for brevity>

Now I wonder why that startup error is output at the end of the test on other platforms. What am I missing?

@iluuu1994, --disable-debug-pack will result in an executable without any associated debug info (.pdb). You can use --disable-debug-pack --enable-debug to have a full fledged debug build. Running such builds in CI might make sense, especially if the envvar PHP_WIN32_DEBUG_HEAP would be set when running the tests, since this gives some basic heap checking. ASAN would be much better, though.

[nielsdos]

@cmb69

Now I wonder why that startup error is output at the end of the test on other platforms. What am I missing?

I think I figured it out. It's because startup errors go though php_log_err_with_severity which will call this function on CLI:

php-src/sapi/cli/php_cli.c

Lines 350 to 356 in a7bd911

	static void sapi_cli_log_message(const char *message, int syslog_type_int) /* {{{ */
	{
	fprintf(stderr, "%s\n", message);
	#ifdef PHP_WIN32
	fflush(stderr);
	#endif
	}

Notice that the buffer is flushed on Windows but not on other platforms. But the normal test output is not sent through stderr, it is sent to stdout. So that's why we end up with different ordering: stderr is already flushed on Windows but not on other platforms so on Windows it appears before stdout and on other platforms it appears after.

[cmb69]

@nielsdos, good thinking, but apparently sapi_cli_log_message() isn't even called, since the test is supposed to be run via php-cgi. Still a stdout vs. stderr issue may be the problem, although stderr is unbuffered on POSIX systems, if I remember correctly (and as such the special case on Windows would be supposed to trigger roughly the same behavior).

Anyway, since debugging PHPTs is harder than simple PHP files (although debugging PHPTs is supported by the php-sdk), I came up with the following simplification of ext/session/tests/bug36459.phpt: x64\Debug\php-cgi.exe -n -d html_errors=0 -d session.use_trans_sid=1 -d session.use_cookies=0 -d session.use_only_cookies=0 -d session.name=sid -f ext\session\tests\bug36459.php, where bug36459.php contains just the --FILE-- section of the PHPT. On Windows this outputs:

Deprecated: PHP Startup: Disabling session.use_only_cookies INI setting is deprecated in Unknown on line 0

Deprecated: PHP Startup: Enabling session.use_trans_sid INI setting is deprecated in Unknown on line 0

Warning: session_start(): Session cache limiter cannot be sent after headers have already been sent in C:\php-sdk\phpdev\vs17\x64\php-src\ext\session\tests\bug36459.php on line 4
<html>
  <head>
    <title>Bug #36459 Incorrect adding PHPSESSID to links, which contains \r\n</title>
  </head>
  <body>
    <p>See source html code</p>
    <a href="/b2w/www/ru/adm/pages/?action=prev&rec_id=8&pid=2&sid=eb20fcb52d5932c00460b5d72fcbd460"
       style="font: normal 11pt Times New Roman">incorrect link</a><br />
    <br />
    <a href="/b2w/www/ru/adm/pages/?action=prev&rec_id=8&pid=2&sid=eb20fcb52d5932c00460b5d72fcbd460" style="font: normal 11pt Times New Roman">correct link</a>
  </body>
</html>

I get the same output with 2>&1 appended to the command (which is what run-tests.php does) on Windows.

I'm now going to build this PR on WSL, to be able to compare what is going on. Haven't done WSL builds for quite a while – might be fun. :)

[nielsdos]

Right, the cgi one is here

php-src/sapi/cgi/cgi_main.c

Line 757 in 0c0da80

static void sapi_cgi_log_message(const char *message, int syslog_type_int)

And it doesn't explicitly flush on Windows.

[cmb69]

Suggested change

	php_error_docref(NULL, E_DEPRECATED, "Disabling session.use_only_cookies INI setting is deprecated");
	php_error_docref("session.configuration", E_DEPRECATED, "Disabling session.use_only_cookies INI setting is deprecated");

Same for the other two below. See #15505.

[kamil-tekiela]

Now back to the issue at hand. To enforce the same behavior on all platforms, we could simply add display_startup_errors=0 at the beginning of the --INI-- sections of the affected tests. Of course, the deprecation warning at the end of the --EXPECT-- sections would need to be removed to let the tests pass. To also check for the deprecation warning, the --EXPECTHEADERS-- hack I suggested above for Windows would not work on POSIX platforms, since the lines are terminated by LF there, and as such would not be detected as headers by run-tests.php. So either we could introduce something like --EXPECTSTARTUPERRORS--, or just ignore the startup errors in these tests, since they are already tested by other tests. I suggest the latter to not have to spend even more time on this issue.

Is what I did in ad35e68 what you had in mind but for all tests in this PR?

[cmb69]

Is what I did in ad35e68 what you had in mind but for all tests in this PR?

Right; well, for all tests which requires it (likely all rfc1867_*.phpts).

[kamil-tekiela]

I don't think I can do that with ext/session/tests/rfc1867_sid_invalid.phpt. If I disable startup errors then the whole test is borked.

[cmb69]

I don't think I can do that with ext/session/tests/rfc1867_sid_invalid.phpt. If I disable startup errors then the whole test is borked.

Ah, interesting. I'll have a closer look at this one soon.

[cmb69]

Ah, interesting. I'll have a closer look at this one soon.

That test uses output buffering and manual session start to work around the startup error issue. But for this test, I don't see any good reason why we would need session.use_only_cookies=0, so I suggest to remove it.

[cmb69]

Besides the two nits, this looks good to me. Thanks for your patience! :)

But let's wait what others have to say (still almost a week till PHP 8.4.0beta4).

[cmb69]

Note that the respective RFC has been accepted unanimously (29:0 votes); it would be a shame if it wouldn't make it into PHP 8.4.

[kamil-tekiela]

@NattyNarwhal Can we merge it as is?

[NattyNarwhal]

makes sense to merge to me