ext/dba/inifile: Do some refactoring

[Girgias]

This is an attempt to remove some PHPism/ZENDism from the library

[nielsdos]

Failures are legit, looks like some warnings that got turned into errors.

[nielsdos]

Suggested change

	return false;
	return true;

because it was SUCCESS; the fact this was not caught by tests is not good :-(

[Girgias]

I'll try to see if I can come up with a test case

[nielsdos]

wait shouldn't this variable (and the function's return value) be zend_result?

[Girgias]

Yes, it should, but I feel ideally this should just return bool, but I would have needed to go and change stuff at the DBA driver level which I wanted to do in a follow-up.

[Girgias]

@cmb69 do you have an idea how I can fix the permission issue I am hitting on Windows?

[cmb69]

@Girgias, just tried running the test locally with a x86 build and got:

Assertion failed: ((uintptr_t)(accel_shared_globals->interned_strings).start & 0x7) == 0, file C:\php-sdk\phpdev\vs17\x86\php-src\ext\opcache\ZendAccelerator.c, line 289

Running the test without OPcache appears to hang.

Running the test with an unpatched master x64 build gives the same test failure as CI. I'll have a closer look.

[cmb69]

 ext/dba/tests/dba_inifile_group_keys.phpt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ext/dba/tests/dba_inifile_group_keys.phpt b/ext/dba/tests/dba_inifile_group_keys.phpt
index eb30527574..b435c44f68 100644
--- a/ext/dba/tests/dba_inifile_group_keys.phpt
+++ b/ext/dba/tests/dba_inifile_group_keys.phpt
@@ -12,7 +12,7 @@
 require_once __DIR__ . '/setup/setup_dba_tests.inc';
 $name = __DIR__ . '/inifile_group_keys_tests.db';
 
-$db = dba_open($name, 'c', 'inifile');
+$db = dba_open($name, 'c-', 'inifile');
 
 echo "Checks on an empty database:\n";
 var_dump(dba_delete(['group', 'name'], $db));

makes the test pass; I wonder why there is a lock on Windows, but apparently not on other platforms?

[Girgias]

Considering the state of this library, I don't know, might be something related to default OS behaviours? But thank you!

[cmb69]

[…], might be something related to default OS behaviours?

D'oh! Windows always uses mandatory locking. :)

I'm still somewhat concerned about the assertion failure on x86 builds. That may or may not be Windows specific, and 32bit builds are likely not that important anymore, but still this should be addressed. It's not related to this PR, though.

[Girgias]

Probably makes sense opening a new issue then to track this?

[nielsdos]

So some code was not error-checked, and so you removed the error-returning code. However, why didn't you add error checks instead?
Also, please try to avoid force pushes with a rebase, it isn't necessary here and it makes it difficult to see what's actually changed since last review.

[Girgias]

I rebased and force-pushed so that the test is the first commit, which I used to check what the behaviour of the current implementation is.

None of the other commits were amended.

I'm not sure that I understand the part about the error checking that I removed? As far as I can see, I didn't do any of that?

[cmb69]

off topic replies

Probably makes sense opening a new issue then to track this?

I'll have a look at this right away.

Running the test without OPcache appears to hang.

This might be because when shell_exec() fails, it returns false or null (the latter causing a deprecation notice), and the following code isn't up to handle an empty result anyway.

[nielsdos]

I rebased and force-pushed so that the test is the first commit, which I used to check what the behaviour of the current implementation is.

None of the other commits were amended.

Thanks for clarifying. The thing is that I can't see that easily when hitting the "compare" button, which is why I brought it up.

I'm not sure that I understand the part about the error checking that I removed? As far as I can see, I didn't do any of that?

No, you didn't remove error checking. My point is this: inifile_find_group used to return success or failure, but this value was never checked. You then removed the success/failure indication. This code is equivalent, so that change on its own is correct. However, the question is whether it's a better idea to actually add the error checks for inifile_find_group. Intuitively this seems like it makes the most sense?