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

Fixup player profile getters and constructor to expected nullability #9770

Merged
merged 3 commits into from
Oct 5, 2023

Conversation

kennytv
Copy link
Member

@kennytv kennytv commented Sep 28, 2023

Changing nullability here would go beyond just getters with its creation and completion, so that feels more like a future thing

@kennytv kennytv requested a review from a team as a code owner September 28, 2023 02:41
@Lulu13022002
Copy link
Contributor

I think the method Server#createProfileExact should also be updated, the method specify than having one of its parameter null is valid. However when the method will find an online player and one of its parameter is null, the constructor will throw. For example: createProfileExact(null, p.getName()), createProfileExact(p.getUniqueId(), null)

@WindSpiritSR
Copy link

@Lulu13022002

Copy link
Contributor

@Lulu13022002 Lulu13022002 left a comment

Choose a reason for hiding this comment

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

The two linked issues above seems relevant and could be fixed here (even if it's a spigot one). The method SkullMeta#setOwningPlayer didn't throw before with an offline player without a name:

stack.editMeta(SkullMeta.class, meta -> {
    meta.setOwningPlayer(Bukkit.getOfflinePlayer(UUID.fromString("a8179ff3-c201-4a75-bdaa-9d14aca6f83f")));
});

Also this patch need to be reevaluate (https://hub.spigotmc.org/stash/projects/SPIGOT/repos/spigot/browse/CraftBukkit-Patches/0055-Fix-Player-Banning.patch): cause it can throw in the case it's still relevant

@@ -103,16 +103,17 @@ index 0000000000000000000000000000000000000000..e513ce5bcb5070b8a57737228904381f
+ @Nullable
+ @Override
+ public UUID getId() {
+ return profile.getId();
+ return profile.getId() != Util.NIL_UUID ? profile.getId() : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think doing an equals instead of a ref equality for the uuid check would be better to have a consistent state across the server since vanilla and spigot does that as well.

@kennytv kennytv merged commit 431e641 into master Oct 5, 2023
2 checks passed
@kennytv kennytv deleted the profile branch October 5, 2023 05:31
@kennytv
Copy link
Member Author

kennytv commented Oct 5, 2023

The other issues are better fixed in another pr/of lesser priority

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

3 participants