QA - pcntl_getpriority_error.phpt - fix test context information and case

[juan-morales]

I changed an existing test as the information inside it is not accurate with what the test is testing.

Also added on more case for non-existing process id provided, in order to add more coverage also like image below shows

[juan-morales]

I am confuse at this.

Fails on MACOS like ...

But how is this possible? In the test code I am inside a try catch that is working everywhere except in Mac ... bug?

[cmb69]

@juan-morales, the macOS failure is for pcntl_getpriority(-123);, which is not inside a try-catch construct. And obviously, there is a bug in our pcntl_getpriority() implementation; at least the error message makes no sense. From the spec:

[EINVAL]
The value of the which argument was not recognized, or the value of the who argument is not a valid process ID, process group ID, or user ID.

And our pcntl_getpriority() implementation mixes up the parameter names …

[juan-morales]

@cmb69 in order to fix this, a change is needed in the C code right? ... if it is lie this ... I will give a try trying to fix it ... but I am new on this field, lets see if I am lucky

[cmb69]

in order to fix this, a change is needed in the C code right?

Yes. The error message needs to be changed:

php-src/ext/pcntl/pcntl.c

Line 1276 in 9f8e518

zend_argument_value_error(2, "must be one of PRIO_PGRP, PRIO_USER, or PRIO_PROCESS");

The only real issue is that we probably can no longer use zend_argument_value_error(), since the problem may be caused by the first parameter. And if it is caused by the first parameter, we shouldn't throw a ValueError at all (because the user is not supposed to know in advance which PIDs are good and which are not). So we probably want to demote to a warning, i.e. basically like the ESRCH case a few lines above.

[juan-morales]

in order to fix this, a change is needed in the C code right?

Yes. The error message needs to be changed:

php-src/ext/pcntl/pcntl.c

Line 1276 in 9f8e518

zend_argument_value_error(2, "must be one of PRIO_PGRP, PRIO_USER, or PRIO_PROCESS");

The only real issue is that we probably can no longer use zend_argument_value_error(), since the problem may be caused by the first parameter. And if it is caused by the first parameter, we shouldn't throw a ValueError at all (because the user is not supposed to know in advance which PIDs are good and which are not). So we probably want to demote to a warning, i.e. basically like the ESRCH case a few lines above.

I am open to do my best and try to work on this, but I dont know exactly the procedure to follow, in terms of ... how to propose such a change (warning instead of error, etc.).

Any kind of advice is super welcome.

[cmb69]

You can "just" file a pull request. :)

[juan-morales]

@cmb69 I have an idea regarding this (and also setpriority() ) after reading documentation. I think I CAN handle this.

I Will have a PR ready soon this days.

[juan-morales]

will be fix in another PR