minetest.get_connected_players should not return nil

[Wuzzy2]

Minetest version

0cb34ce158989b66ecd42257dbed6729a338ff1e

Summary

If you all minetest.get_connected_players() during loadtime, it returns nil. This is a regression from 5.1.1.

lua_api.txt of 5.1.1 clearly states:

* `minetest.get_connected_players()`: returns list of `ObjectRefs`

Expected behavior

It returns {} on loadtime, since, in fact, no players are connected yet.

[paramat]

'nil' seems suitable because no players are connected. What was the previous behaviour? Will it break mods if it has changed from {}?
@sfan5

[rubenwardy]

Yes, this has broken at least "mobs", "mineclone2" and "crafting" so far.

Given that this function is in "misc" and not "environment" in the documentation, and there is no indication that this does not work at load time, then I would consider this a break in behaviour

See the related discussion in #minetest-dev: http://irc.minetest.net/minetest-dev/2020-03-09#i_5649975

[Wuzzy2]

I would very much prefer it continues to return {}. It's more logical and makes for simpler code. Why change the data type just because the number of connected players is 0?

[rubenwardy]

Why change the data type just because the number of connected players is 0?

It's because the server (and so the environment/world) isn't initialised yet. If there are no players connected and the server isn't currently loading then {} will be returned

[sfan5]

Why change the data type just because the number of connected players is 0?

Returning nil makes more sense in the C++ code. That part is also hidden inside the GET_ENV_PTR:

Also this issue is a duplicate of #9428.

[sfan5]

To the actual issue: I'm okay with making it return {} instead in the interest of not breaking mods (though I don't mind the current behaviour either).

[paramat]

Ok, to be clear i have no objection to {}, obviously (to not break stuff).

[sorcerykid]

I would very much prefer it continues to return {}. It's more logical and makes for simpler code. Why change the data type just because the number of connected players is 0?

Because the number of players is NOT actually zero, There cannot be players at load time. Why any mod would query a list of players (or do anything server-related) when the server is not in a consistent state is completely baffling. This shouldn't even have to be documented, since it's common sense that these API functions might eventually fail. The fact that they even worked up till now is a fluke.

The only thing that should happen at load time is to perform registrations, initialize variables, setup data structures, read settings and configuration, etc.

On a sidenote, I find it curious how "bad programming practices" are now permissible when one or more core devs finds it impacts them. Then it's an exception that must be specially accommodated by the engine. Even though in the past, core devs have strongly opposed making any changes to the engine that specifically encourage bad programming practices.

[rubenwardy]

We are not permitting this, we are logging a deprecated warning rather than crashing mods. In a few releases we can then change the behaviour (I'm for a rule where removing something isn't a breaking change if there has been at least one release which has a deprecation warning)

[sorcerykid]

Okay, that's great news! The deprecation warning is in-line with the suggestion I made yesterday on the forums. I had no idea that was the resolution, since there was no mention of option for a deprecation warning in Wuzzy's original post, only restoring the original behavior. So that's why I was so concerned. Thanks for clarifying :)

[rubenwardy]

Oh, I forgot that the op didn't mention that