Skip to content

Fix enchant_broker_set_dict_path() and enchant_broker_get_dict_path() returning null - #23307

Open
lacatoire wants to merge 1 commit into
php:masterfrom
lacatoire:fix/enchant-dict-path-return-value
Open

Fix enchant_broker_set_dict_path() and enchant_broker_get_dict_path() returning null#23307
lacatoire wants to merge 1 commit into
php:masterfrom
lacatoire:fix/enchant-dict-path-return-value

Conversation

@lacatoire

Copy link
Copy Markdown
Member

enchant_broker_set_dict_path() and enchant_broker_get_dict_path() return null on every build linked against libenchant 2, violating the bool and string|false return 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.m4 defines that macro only on the legacy libenchant 1.x path, and its own comment says why: enchant_broker_set_param was "available since 1.5.0 and removed in 2.x". Since PKG_CHECK_MODULES looks for enchant-2 first and that is what distributions ship, both bodies are compiled away in practice.

Reproduced from php:8.4-cli with libenchant-2-dev and docker-php-ext-install enchant, libenchant 2.8.2:

<?php
$b = enchant_broker_init();
var_dump(@enchant_broker_set_dict_path($b, ENCHANT_MYSPELL, '/tmp'));  // NULL, declared bool
var_dump(@enchant_broker_get_dict_path($b, ENCHANT_MYSPELL));          // NULL, declared string|false

Internal functions are not return-type checked, so this stays silent until the value reaches typed userland code, where the TypeError points at the caller rather than at the source.

Returning false on that path matches what both functions already do for every in-band failure: unknown $type, empty path, unset path.

enchant_broker_set_dict_path.phpt and bug53070.phpt both carry skip 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.

… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant