Fix default item callbacks to work with nil users (#5819)

* Fix default item callbacks to work with nil users

* item.lua: Handle node drops for invalid players

The if-condition for the dropping loop is the same as `inv`, which means that the 2nd possible definition of `give_item` is never used.
Remove redundant `local _, dropped_item`

builtin/game/chatcommands.lua

@@ -667,8 +667,8 @@ core.register_chatcommand("pulverize", {
 core.rollback_punch_callbacks = {}
 
 core.register_on_punchnode(function(pos, node, puncher)
-	local name = puncher:get_player_name()
+	local name = puncher and puncher:get_player_name()
-	if core.rollback_punch_callbacks[name] then
+	if name and core.rollback_punch_callbacks[name] then
 		core.rollback_punch_callbacks[name](pos, node, puncher)
 		core.rollback_punch_callbacks[name] = nil
 	end

builtin/game/item.lua

@@ -215,6 +215,8 @@ function core.get_node_drops(node, toolname)
 		end
 		if item.tools ~= nil then
 			good_tool = false
+		end
+		if item.tools ~= nil and toolname then
 			for _, tool in ipairs(item.tools) do
 				if tool:sub(1, 1) == '~' then
 					good_tool = toolname:find(tool:sub(2)) ~= nil
@@ -245,6 +247,20 @@ function core.get_node_drops(node, toolname)
 	return got_items
 end
 
+local function user_name(user)
+	return user and user:get_player_name() or ""
+end
+
+local function is_protected(pos, name)
+	return core.is_protected(pos, name) and
+		not minetest.check_player_privs(name, "protection_bypass")
+end
+
+-- Returns a logging function. For empty names, does not log.
+local function make_log(name)
+	return name ~= "" and core.log or function() end
+end
+
 function core.item_place_node(itemstack, placer, pointed_thing, param2)
 	local def = itemstack:get_definition()
 	if def.type ~= "node" or pointed_thing.type ~= "node" then
@@ -255,10 +271,11 @@ function core.item_place_node(itemstack, placer, pointed_thing, param2)
 	local oldnode_under = core.get_node_or_nil(under)
 	local above = pointed_thing.above
 	local oldnode_above = core.get_node_or_nil(above)
-	local playername = placer:get_player_name()
+	local playername = user_name(placer)
+	local log = make_log(playername)
 
 	if not oldnode_under or not oldnode_above then
-		core.log("info", playername .. " tried to place"
+		log("info", playername .. " tried to place"
 			.. " node in unloaded position " .. core.pos_to_string(above))
 		return itemstack, false
 	end
@@ -269,7 +286,7 @@ function core.item_place_node(itemstack, placer, pointed_thing, param2)
 	olddef_above = olddef_above or core.nodedef_default
 
 	if not olddef_above.buildable_to and not olddef_under.buildable_to then
-		core.log("info", playername .. " tried to place"
+		log("info", playername .. " tried to place"
 			.. " node in invalid position " .. core.pos_to_string(above)
 			.. ", replacing " .. oldnode_above.name)
 		return itemstack, false
@@ -280,13 +297,12 @@ function core.item_place_node(itemstack, placer, pointed_thing, param2)
 
 	-- If node under is buildable_to, place into it instead (eg. snow)
 	if olddef_under.buildable_to then
-		core.log("info", "node under is buildable to")
+		log("info", "node under is buildable to")
 		place_to = {x = under.x, y = under.y, z = under.z}
 	end
 
-	if core.is_protected(place_to, playername) and
+	if is_protected(place_to, playername) then
-			not minetest.check_player_privs(placer, "protection_bypass") then
+		log("action", playername
-		core.log("action", playername
 				.. " tried to place " .. def.name
 				.. " at protected position "
 				.. core.pos_to_string(place_to))
@@ -294,7 +310,7 @@ function core.item_place_node(itemstack, placer, pointed_thing, param2)
 		return itemstack
 	end
 
-	core.log("action", playername .. " places node "
+	log("action", playername .. " places node "
 		.. def.name .. " at " .. core.pos_to_string(place_to))
 
 	local oldnode = core.get_node(place_to)
@@ -314,7 +330,7 @@ function core.item_place_node(itemstack, placer, pointed_thing, param2)
 	-- Calculate the direction for furnaces and chests and stuff
 	elseif (def.paramtype2 == "facedir" or
 			def.paramtype2 == "colorfacedir") and not param2 then
-		local placer_pos = placer:getpos()
+		local placer_pos = placer and placer:getpos()
 		if placer_pos then
 			local dir = {
 				x = above.x - placer_pos.x,
@@ -322,7 +338,7 @@ function core.item_place_node(itemstack, placer, pointed_thing, param2)
 				z = above.z - placer_pos.z
 			}
 			newnode.param2 = core.dir_to_facedir(dir)
-			core.log("action", "facedir: " .. newnode.param2)
+			log("action", "facedir: " .. newnode.param2)
 		end
 	end
 
@@ -348,7 +364,7 @@ function core.item_place_node(itemstack, placer, pointed_thing, param2)
 	-- Check if the node is attached and if it can be placed there
 	if core.get_item_group(def.name, "attached_node") ~= 0 and
 		not builtin_shared.check_attached_node(place_to, newnode) then
-		core.log("action", "attached node " .. def.name ..
+		log("action", "attached node " .. def.name ..
 			" can not be placed at " .. core.pos_to_string(place_to))
 		return itemstack, false
 	end
@@ -419,26 +435,25 @@ function core.item_secondary_use(itemstack, placer)
 end
 
 function core.item_drop(itemstack, dropper, pos)
-	if dropper and dropper:is_player() then
+	local dropper_is_player = dropper and dropper:is_player()
-		local dir = dropper:get_look_dir()
+	local p = table.copy(pos)
-		local p = {x = pos.x, y = pos.y + 1.2, z = pos.z}
 	local cnt = itemstack:get_count()
+	if dropper_is_player then
+		p.y = p.y + 1.2
+	end
 	local item = itemstack:take_item(cnt)
 	local obj = core.add_item(p, item)
 	if obj then
+		if dropper_is_player then
+			local dir = dropper:get_look_dir()
 			dir.x = dir.x * 2.9
 			dir.y = dir.y * 2.9 + 2
 			dir.z = dir.z * 2.9
 			obj:set_velocity(dir)
 			obj:get_luaentity().dropped_by = dropper:get_player_name()
-			return itemstack
 		end
-
-	else
-		if core.add_item(pos, itemstack) then
 		return itemstack
 	end
-	end
 	-- If we reach this, adding the object to the
 	-- environment failed
 end
@@ -458,7 +473,8 @@ function core.do_item_eat(hp_change, replace_with_item, itemstack, user, pointed
 				itemstack:add_item(replace_with_item)
 			else
 				local inv = user:get_inventory()
-				if inv:room_for_item("main", {name=replace_with_item}) then
+				-- Check if inv is null, since non-players don't have one
+				if inv and inv:room_for_item("main", {name=replace_with_item}) then
 					inv:add_item("main", replace_with_item)
 				else
 					local pos = user:getpos()
@@ -473,8 +489,10 @@ end
 
 function core.item_eat(hp_change, replace_with_item)
 	return function(itemstack, user, pointed_thing)  -- closure
+		if user then
 			return core.do_item_eat(hp_change, replace_with_item, itemstack, user, pointed_thing)
 		end
+	end
 end
 
 function core.node_punch(pos, node, puncher, pointed_thing)
@@ -490,10 +508,20 @@ end
 
 function core.handle_node_drops(pos, drops, digger)
 	-- Add dropped items to object's inventory
-	if digger:get_inventory() then
+	local inv = digger and digger:get_inventory()
-		local _, dropped_item
+	local give_item
-		for _, dropped_item in ipairs(drops) do
+	if inv then
-			local left = digger:get_inventory():add_item("main", dropped_item)
+		give_item = function(item)
+			return inv:add_item("main", item)
+		end
+	else
+		give_item = function(item)
+			return item
+		end
+	end
+
+	for _, dropped_item in pairs(drops) do
+		local left = give_item(dropped_item)
 		if not left:is_empty() then
 			local p = {
 				x = pos.x + math.random()/2-0.25,
@@ -503,35 +531,36 @@ function core.handle_node_drops(pos, drops, digger)
 			core.add_item(p, left)
 		end
 	end
-	end
 end
 
 function core.node_dig(pos, node, digger)
+	local diggername = user_name(digger)
+	local log = make_log(diggername)
 	local def = core.registered_nodes[node.name]
 	if def and (not def.diggable or
 			(def.can_dig and not def.can_dig(pos, digger))) then
-		core.log("info", digger:get_player_name() .. " tried to dig "
+		log("info", diggername .. " tried to dig "
 			.. node.name .. " which is not diggable "
 			.. core.pos_to_string(pos))
 		return
 	end
 
-	if core.is_protected(pos, digger:get_player_name()) and
+	if is_protected(pos, diggername) then
-			not minetest.check_player_privs(digger, "protection_bypass") then
+		log("action", diggername
-		core.log("action", digger:get_player_name()
 				.. " tried to dig " .. node.name
 				.. " at protected position "
 				.. core.pos_to_string(pos))
-		core.record_protection_violation(pos, digger:get_player_name())
+		core.record_protection_violation(pos, diggername)
 		return
 	end
 
-	core.log('action', digger:get_player_name() .. " digs "
+	log('action', diggername .. " digs "
 		.. node.name .. " at " .. core.pos_to_string(pos))
 
-	local wielded = digger:get_wielded_item()
+	local wielded = digger and digger:get_wielded_item()
-	local drops = core.get_node_drops(node, wielded:get_name())
+	local drops = core.get_node_drops(node, wielded and wielded:get_name())
 
+	if wielded then
 		local wdef = wielded:get_definition()
 		local tp = wielded:get_tool_capabilities()
 		local dp = core.get_dig_params(def and def.groups, tp)
@@ -547,6 +576,7 @@ function core.node_dig(pos, node, digger)
 			end
 		end
 		digger:set_wielded_item(wielded)
+	end
 
 	-- Handle drops
 	core.handle_node_drops(pos, drops, digger)

doc/lua_api.txt

@@ -2444,6 +2444,7 @@ Call these functions only at load time!
 * `minetest.register_on_placenode(func(pos, newnode, placer, oldnode, itemstack, pointed_thing))`
     * Called when a node has been placed
     * If return `true` no item is taken from `itemstack`
+    * `placer` may be any valid ObjectRef or nil.
     * **Not recommended**; use `on_construct` or `after_place_node` in node definition
       whenever possible
 * `minetest.register_on_dignode(func(pos, oldnode, digger))`
@@ -3219,6 +3220,7 @@ These functions return the leftover itemstack.
     * Returns true, if player `name` shouldn't be abled to dig at `pos` or do other
       actions, defineable by mods, due to some mod-defined ownership-like concept.
       Returns false or nil, if the player is allowed to do such actions.
+    * `name` will be "" for non-players or unknown players.
     * This function should be overridden by protection mods and should be used to
       check if a player can interact at a position.
     * This function should call the old version of itself if the position is not
@@ -4608,6 +4610,7 @@ Definition tables
         ^ Called after constructing node when node was placed using
           minetest.item_place_node / minetest.place_node
         ^ If return true no item is taken from itemstack
+	^ `placer` may be any valid ObjectRef or nil
         ^ default: nil ]]
         after_dig_node = func(pos, oldnode, oldmetadata, digger), --[[
         ^ oldmetadata is in table format

games/minimal/mods/default/init.lua

@@ -1270,7 +1270,9 @@ minetest.register_node("default:chest_locked", {
 	sounds = default.node_sound_wood_defaults(),
 	after_place_node = function(pos, placer)
 		local meta = minetest.get_meta(pos)
-		meta:set_string("owner", placer:get_player_name() or "")
+		local pname =
+			placer and placer:get_player_name() or ""
+		meta:set_string("owner", pname)
 		meta:set_string("infotext", "Locked Chest (owned by "..
 				meta:get_string("owner")..")")
 	end,