ext/gettext: dcgettext/dcngettext sigabrt on macOs. #13555
Conversation
devnexen
force-pushed
the
fix_gettext_0225_abort
branch
4 times, most recently
from
a4d7f0d to
f5215c9
Compare
|
It looks like the test has been interrupted, so you might want to force push again. |
devnexen
force-pushed
the
fix_gettext_0225_abort
branch
from
f5215c9 to
c422d09
Compare
the man page states `the locale facet is determined by the category argument, which should be one of the LC_xxx constants defined in the <locale.h> header, excluding LC_ALL`, since the 0.22.5 release, sanity checks had been strenghtened leading to an abort with the Zend/tests/arginfo_zpp_mismatch.phpt test setting the category to 0 which is LC_ALL on macOs.
devnexen
force-pushed
the
fix_gettext_0225_abort
branch
from
c422d09 to
183f51d
Compare
There was a problem hiding this comment.
Personally: LGTM, very nice catch!
However, adding a new exception in existing releases is an extremely rare situation... so I'm wondering if it would be worth to only add it for v0.22.5+ in case of PHP 8.3 and below. On the other hand, master could throw an exception for each gettext version of course. Inviting @Girgias and @bukka for discussion.
|
Throwing in a particular gettext version would require an API to recognize the version at runtime, because the library is usually dynamically linked. Unfortunately I couldn't find such an API. If BC breaks are a concern, we may merge this only for master. However, note that segfaults or undefined behavior are likely worse than an exception, which will happen once people update gettext. Alternatively we may try to simulate the old behavior. I'm not really familiar with gettext, so I don't know if that's possible. |
|
This change might not be too impactful ; ideally users already put those calls into a try/catch block since it can already throw for other reasons |
|
What was the previous behavior? I mean what happens in the previous version of gettext if LC_ALL provided? |
|
Not really comfortable with this going to stable branches as RM... |
let's say the functions did not follow the contract, trying with 0.21 the call does return the msgid as no conversion occurs but does not trigger any error neither. |
|
So what is being returned? I mean what is in msgstr? empty string? |
|
I'm just on my mac and lazy to get older gettext version installed :) |
|
Also what do we get on other platforms? |
|
Basically I think we should keep the behavior as it was in stable versions. I'm fine with exception in master though |
in this case then yes empty string. |
what do you suggest ? if LC_ALL return msgid (not calling the function at all) ? |
|
yeah exactly, return empty string if LC_ALL without even calling the function and just do the exception in master with note in UPGRADING. |
the man page states
the locale facet is determined by the category argument, which should be one of the LC_xxx constants defined in the <locale.h> header, excluding LC_ALL,since the 0.22.5 release, sanity checks had been strenghtened leading to an abort with the Zend/tests/arginfo_zpp_mismatch.phpt test setting the category to 0 which is LC_ALL on macOs.