Skip to content

Fix enchant stub #5500

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fix enchant stub #5500

wants to merge 1 commit into from

Conversation

cmb69
Copy link
Member

@cmb69 cmb69 commented Apr 30, 2020

enchant_broker_list_dicts() and enchant_broker_describe() return
null if no broker is available.


Alternatively, we could change the implementation to return an empty array in this case.

`enchant_broker_list_dicts()` and `enchant_broker_describe()` return
`null` if no broker is available.
@cmb69 cmb69 added the Quickfix label May 2, 2020
@cmb69
Copy link
Member Author

cmb69 commented May 5, 2020

@remicollet, what do you think?

@nikic
Copy link
Member

nikic commented May 5, 2020

Does it really return null? PHP_ENCHANT_GET_BROKER returns false if no broker available (it should throw instead).

@cmb69
Copy link
Member Author

cmb69 commented May 5, 2020

return_value is only initialized inside of the callback which traverses the brokers/dicts. If there are none, the callback is never executed, so return_value stays undefined.

@nikic
Copy link
Member

nikic commented May 5, 2020

@cmb69 I see. In that case I'd say this definitely should array_init(return_value) beforehand.

@remicollet
Copy link
Member

@cmb69 I agree with your PR, change NULL to empty array may be a BC break.

@nikic yes PHP_ENCHANT_GET_BROKER could raise an exception, but this only happens is user call enchant_broker_free-*, thus the reason why I deprecate these in pr #5528

@remicollet
Copy link
Member

remicollet commented May 5, 2020

@cmb69 I merged this PR in mine

@nikic in pr #5528 invalid object now raise an exception

@nikic
Copy link
Member

nikic commented May 5, 2020

@cmb69 I agree with your PR, change NULL to empty array may be a BC break.

It's ok for PHP 8.

@cmb69
Copy link
Member Author

cmb69 commented May 5, 2020

I agree that the minor BC break would be acceptable for PHP 8, especially since this is only relevant if the extension is not really usable (i.e. has no brokers or dicts). On the other hand, I think sticking with ?array is also okay.

Anyhow, @remicollet has incorporated this in PR #5528, so this PR can be closed.

@cmb69 cmb69 closed this May 5, 2020
@cmb69 cmb69 deleted the cmb/enchant-stub branch May 5, 2020 15:57
@nikic
Copy link
Member

nikic commented May 5, 2020

This issue does not seem related to the object migration, so it would be preferable to address it independently.

@cmb69 cmb69 restored the cmb/enchant-stub branch May 6, 2020 08:23
@cmb69
Copy link
Member Author

cmb69 commented May 6, 2020

Okay, makes sense. @remicollet, could you please revert these changes from PR #5528.

@cmb69 cmb69 reopened this May 6, 2020
@remicollet
Copy link
Member

Applied in c1ad916

@remicollet remicollet closed this May 6, 2020
@cmb69 cmb69 deleted the cmb/enchant-stub branch July 11, 2020 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants