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

ASMAPI.getSystemPropertyFlag() checks the given property without only prepending "coremod." #45

Merged

Conversation

Jonathing
Copy link
Member

@Jonathing Jonathing commented Nov 8, 2023

Successor to #41.

Previously, ASMAPI.getSystemPropertyFlag() would prematurely prepend "coremod." to the string before checking it using Boolean.getBoolean(). This PR retains that logic, but also checks if the string itself exists as a true property. The old logic contained has been deleted as it has been deprecated for removal since 5.0 and we are now at 5.1 (and who actually uses this except for me lmao).

ASMAPI only checks if the given JVM property boolean is true, so there is no need to worry about "security" (aside from the fact that we're literally talking about Minecraft of all things) when it comes to getting properties.

@LexManos
Copy link
Member

Can you not call Boolean.getBoolean directly in your coremod? Does this helper need to exist at all?

@Jonathing
Copy link
Member Author

java.lang is not allowed in the sandbox, so I can't invoke the Boolean wrapper class.

@LexManos
Copy link
Member

Ah, ok, well then make implementation not call the old function, that way you dont have to change the logic of the old function. And it makes it easier to delete in the future.
So should just be return getBoolean(foo) || getBoolean(coremods.foo)

@Jonathing Jonathing changed the title ASMAPI.getSystemPropertyFlag() no longer prepends "coremod." ASMAPI.getSystemPropertyFlag() checks the given property without only prepending "coremod." Nov 13, 2023
@LexManos LexManos merged commit 7f46b51 into MinecraftForge:master Nov 13, 2023
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.

2 participants