Skip to content

Don't return null from enchant array APIs #5566

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

Conversation

nikic
Copy link
Member

@nikic nikic commented May 13, 2020

Followup to #5500, which makes sure all array returns are initialized, so the default "null" value does not get returned.

cc @cmb69 @remicollet

@remicollet
Copy link
Member

remicollet commented May 13, 2020

I'm not a big fan with such BC break (and hard to find one)

All people using is_array(...) will have to use count(...) which doesn't work on older version (as false is not countable).

P.S. yes BC break are possible in 8, and yes this seems a better API, btw, if we can avoid it...

@cmb69
Copy link
Member

cmb69 commented May 13, 2020

I'm not sure that the BC break is actually a thing in practise. I think the case that enchant_broker_describe() returns null means there is a broken setup (why having enchant without any back-end?). And even enchant_broker_list_dicts() returning null looks rather unlikely. Furthermore, the PHP manual does not even mention the null return; instead it says the function return an array, and false in case of failure (wereas the failure case is not even specified, but no back-ends/dictionaries don't look like a failure to me).

And those who are looking for PHP 7 and 8 compatibility, could test for empty() or just cast/convert to return value to bool.

So I'm slightly in favor of this change, but am fine if we don't change the behavior.

@nikic
Copy link
Member Author

nikic commented May 13, 2020

I would like to add why I think having array rather than ?array return here is valuable: It allows you to write code like this:

foreach (enchant_broker_list_dicts($broker) as $dict) {
    // ...
}

while right now, at least if you want to handle these special cases (I don't know if they actually happen in practice), you'd have to write something like this:

$dicts = enchant_broker_list_dicts($broker);
if ($dicts) {
    foreach ($dicts as $dict) {
        // ...
    }
}

As we have the opportunity to change this now for PHP 8, I think we should take it.

@nikic
Copy link
Member Author

nikic commented May 20, 2020

Anyone else have an opinion on this? @kocsismate @Girgias maybe.

@kocsismate
Copy link
Member

I can 100% relate to Christoph's reasoning. Not having null documented as a return type + the fact that it's an unlikely case is worth the BC break in order for a good cause :)

@Girgias
Copy link
Member

Girgias commented May 20, 2020

Basing on the other work we did to remove false/null returns to more sensible defaults (emtpy strings, empty arrays, error) I would agree that breaking BC here seems pretty reasonable due to the various other cases doing the same.

@php-pulls php-pulls closed this in a01b6e5 May 27, 2020
@carusogabriel carusogabriel added this to the PHP 8.0 milestone May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants