Fix enchant_broker_set_dict_path() and enchant_broker_get_dict_path() returning null - #23307
Open
lacatoire wants to merge 1 commit into
Open
Fix enchant_broker_set_dict_path() and enchant_broker_get_dict_path() returning null#23307lacatoire wants to merge 1 commit into
lacatoire wants to merge 1 commit into
Conversation
… returning null Both function bodies sit entirely inside #ifdef HAVE_ENCHANT_BROKER_SET_PARAM, with nothing after the #endif. That macro is only defined on the legacy libenchant 1.x path of config.m4, since enchant_broker_set_param() was removed in libenchant 2, so on every build linked against enchant-2 the functions fall off the end and return null, violating their declared bool and string|false return types. Return false on that path instead, which is the value both functions already use for every in-band failure. The two existing tests are skipped on libenchant 2, so the broken path had no coverage; a test is added for it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
enchant_broker_set_dict_path()andenchant_broker_get_dict_path()returnnullon every build linked against libenchant 2, violating theboolandstring|falsereturn types their stubs declare.In both functions the entire body after parameter parsing sits inside
#ifdef HAVE_ENCHANT_BROKER_SET_PARAM, with nothing after the#endif, so when the macro is undefined the function falls off the end without setting a return value.ext/enchant/config.m4defines that macro only on the legacy libenchant 1.x path, and its own comment says why:enchant_broker_set_paramwas "available since 1.5.0 and removed in 2.x". SincePKG_CHECK_MODULESlooks forenchant-2first and that is what distributions ship, both bodies are compiled away in practice.Reproduced from
php:8.4-cliwithlibenchant-2-devanddocker-php-ext-install enchant, libenchant 2.8.2:Internal functions are not return-type checked, so this stays silent until the value reaches typed userland code, where the
TypeErrorpoints at the caller rather than at the source.Returning
falseon that path matches what both functions already do for every in-band failure: unknown$type, empty path, unset path.enchant_broker_set_dict_path.phptandbug53070.phptboth carryskip libenchant v1 only, so the affected path had no coverage. A test is added for it, verified to fail without the C change and pass with it. The whole suite passes on a libenchant 2.3.3 build: 32 passed, 2 skipped, those being the v1-only tests.Both functions are deprecated since 8.0, which argues for this minimal fix rather than a redesign. An explicit diagnostic on the unsupported path would also be defensible, since they currently look like silent no-ops; left out here to keep the change minimal.