Skip to content
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

Address set_player_privs footgun #14297

Merged
merged 2 commits into from Jan 22, 2024
Merged

Conversation

appgurueu
Copy link
Contributor

Straightforward PR to get rid of the set_player_privs footgun. This effectively does three things:

  • Document this more clearly: privs are a set, set_player_privs replaces the privs (unlike set_properties for example, which just changes properties).
  • Add warnings for false values (modder probably meant to revoke), add warning for non-true values (code smell).
  • Add a change_player_privs convenience wrapper so modders have a function they can use to grant/revoke privs without having to call get_player_privs / set_player_privs themselves. I think granting / revoking privileges is probably the most common use case.

How to test

  • minetest.set_player_privs("singleplayer", {interact = "maybe"}) should warn about a non-true value.
  • minetest.set_player_privs("singleplayer", {interact = false}) should warn about granting a privilege rather than revoking it.
  • minetest.change_player_privs("singleplayer", {interact = 42}) should warn.
  • minetest.change_player_privs("singleplayer", {fast = true, fly = false}) should grant fast and revoke fly.

for priv, value in pairs(privileges) do
-- Warnings for improper API usage
if value == false then
core.log('warning', "`false` value in `privs`, this is almost certainly a bug, "..
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be core.log("deprecated"

also I would leave off the "granting a privilege rather than revoking it" part

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with grorp's suggestion.

builtin/game/auth.lua Show resolved Hide resolved
for priv, value in pairs(privileges) do
-- Warnings for improper API usage
if value == false then
core.log('warning', "`false` value in `privs`, this is almost certainly a bug, "..

This comment was marked as resolved.

builtin/game/auth.lua Outdated Show resolved Hide resolved
@appgurueu
Copy link
Contributor Author

By the way, it looks like log_level='deprecated' is not documented in lua_api.md, and it also seems to be slightly broken? (For example I get (at [C]:-1) when I execute this via dbg's /lua command). But that's out of scope for this PR. It's more appropriate than 'warning', though.

@sfan5
Copy link
Member

sfan5 commented Jan 22, 2024

By the way, it looks like log_level='deprecated' is not documented in lua_api.md,

Looks more like an internal engine helper to me. Please don't document it.

For example I get (at [C]:-1) when I execute this via dbg's /lua command

The C code hardcodes 2 as a stack depth, that's why.

Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

builtin/game/auth.lua Outdated Show resolved Hide resolved
Co-authored-by: grorp <gregor.parzefall@posteo.de>
@appgurueu appgurueu merged commit afc48cf into minetest:master Jan 22, 2024
2 checks passed
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.

None yet

5 participants