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

Fix alias handling of get_content_id #9712

Merged
merged 1 commit into from Apr 19, 2020
Merged

Conversation

sfan5
Copy link
Member

@sfan5 sfan5 commented Apr 19, 2020

...also some miscellaneous cleanups in itemdef.cpp
fixes #9632

To do

This PR is a Ready for Review.

How to test

Put the following code in the top level of a mod, not inside a callback or chatcommand.

Test that the following code succeeds:

minetest.register_alias("aliastest1", "air")
assert(minetest.get_content_id("aliastest1") == minetest.get_content_id("air"))

Test that the following code throws an error:

minetest.register_alias("aliastest2", "asdf:asdf")
minetest.get_content_id("aliastest2")

@sfan5 sfan5 added the Bugfix 🐛 PRs that fix a bug label Apr 19, 2020
@sfan5 sfan5 added this to the 5.3.0 milestone Apr 19, 2020
Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

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

Works as expected.

src/itemdef.cpp Outdated Show resolved Hide resolved
src/itemdef.cpp Outdated Show resolved Hide resolved
@sfan5 sfan5 merged commit 338195f into minetest:master Apr 19, 2020
@sfan5 sfan5 deleted the content_id branch April 19, 2020 17:08
@Treer
Copy link
Contributor

Treer commented Apr 19, 2020

Thanks for fixing that!

nerzhul pushed a commit to nerzhul/minetest that referenced this pull request Apr 27, 2020
@HeyITGuyFixIt
Copy link

HeyITGuyFixIt commented Jul 15, 2020

Why would using minetest.get_content_id to get the id of a non existing node result in an error? Wouldn't it be better to simply return with nil, or is there a better way to check for a node's existence? The moresnow mod uses it assuming it will return a value and never an error.

@Treer
Copy link
Contributor

Treer commented Jul 16, 2020

@HeyITGuyFixIt The PR that introduced that change to get_content_id is #9458, and they make the argument for the behaviour change there.

My cloudlands mod was also using it to check for whether nodes exist, so I had to update the lines to this:
if minetest.registered_nodes[name] ~= nil then result = minetest.get_content_id(name) end

Obviously I had found the old behaviour useful, but I must admit that if you ignore the alias handling bug it uncovered (i.e. what this PR fixes) very few mods seem to have been [incorrectly] broken by the new get_content_id behaviour. Checking for the existence of nodes seems to be a surprisingly uncommon practice in Minetest mods - or was being done with the registered_nodes table.

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.

register_alias_force() breaks get_content_id()
5 participants