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 NPE when loadStructure fails #3812

Conversation

TheDGOfficial
Copy link
Contributor

Description

Fixes NPE when loadStructure fails.

The NPE is not from loadScript returning null or not from info.commandNames actually, it is from cfg.getFileName. So this PR makes fileName default to "" (does not matter anyway since loadStructure failed, we do not want to sync commands or such)

Maybe just continue on empty script info or null cfg but that may change behaviour. I've tested this and it correctly sends permission denied errors.


Target Minecraft Versions: any
Requirements: none
Related Issues: #3804

@TPGamesNL
Copy link
Member

IMO it would be better to make sure every method that returns one or more Configs can't return null values (as value or list element). Some methods already do this:

Config cfg = loadStructure(f);
if (cfg != null)
loadedFiles.add(cfg);

but not all:
List<Config> loadedFiles = new ArrayList<>(files.length);
for (final File f : files) {
assert f != null : Arrays.toString(files);
loadedFiles.add(loadStructure(f));
}
return loadedFiles;

@TPGamesNL TPGamesNL added 2.5 bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. labels Mar 13, 2021
@TheDGOfficial
Copy link
Contributor Author

Yes, that would be better. Looking now.

@TheDGOfficial
Copy link
Contributor Author

I just added the null check to the method you've mentioned and the one that causes this error. Don't have time to look into all methods to find all ones that can return arrays/lists with null values though, so let me know if I've missed any.

@TPGamesNL
Copy link
Member

You should look at the usages of ScriptLoader.loadStructure(File), since that's the method that's used everywhere (ScriptLoader, EffScriptFile and SkriptCommand).
PS I said it wrong in my last message, since a method returning a single Config should return null sometimes. I meant to say that all other methods using this should account for that, which they don't.

@TPGamesNL
Copy link
Member

Closing as #3924 has been merged, fixing the same issues

@TPGamesNL TPGamesNL closed this May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants