Skip to content

Conversation

@Andre601
Copy link
Collaborator

Pull Request

Type

  • Internal change (Doesn't affect end-user).
  • External change (Does affect end-user).
  • Wiki (Changes towards the Wiki).
  • Other: __________

Description

Closes #864

Fixes an issue where the same expansion would be loaded twice, if the jar file used has a slightly different name.

Now PlaceholderAPI checks if the used identifier is already registered and in such a case prints a warning and doesn't load the expansion.

@Andre601 Andre601 added Target: Code This issue/PR is targeting the Code of PlaceholderAPI Type: Bugfix This fixes an existing bug in PlaceholderAPI. labels Jul 30, 2022
Copy link
Member

@BlitzOffline BlitzOffline left a comment

Choose a reason for hiding this comment

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

The only thing I'd really change is the message.

From Failed to load expansion %s. Identifier is already registered.
To Failed to load expansion %s because another expansion with the same identifier is already registered.

Since the identifier isn't registered, the expansion with that identifier is.

@Andre601
Copy link
Collaborator Author

Andre601 commented Feb 20, 2023

Feels a bit wordy...
Perhaps Failed to load expansion '%s'. Identifier '%s' is already in use.

@darbyjack darbyjack merged commit d5c3710 into master Mar 17, 2023
@Starmism Starmism deleted the fix/duplicate-expansion-loading branch March 17, 2023 18:19
@Frcsty
Copy link
Contributor

Frcsty commented Mar 21, 2023

Request to revert this merge as the actual identifier for the expansion should never be changed. Which in turn causes stuff to break when reloading.

@Andre601
Copy link
Collaborator Author

Request to revert this merge as the actual identifier for the expansion should never be changed. Which in turn causes stuff to break when reloading.

?

@iGabyTM
Copy link
Member

iGabyTM commented Mar 21, 2023

Request to revert this merge as the actual identifier for the expansion should never be changed. Which in turn causes stuff to break when reloading.

Do you have proof that reload doesn't work or is just a guess? Iirc the expansions map is cleared on reload.
Edit: Actually, I think you might talk about this

Edit no.2: PlaceholderExpansion#persist() should be true only for expansions registered by plugins, from within their jar, and not for expansions loaded from the expansions folder. Unlike local expansions, the ones provided by plugins can't be updated anyways unless you restart the server, so it is fine for them to be persistent.

@Frcsty
Copy link
Contributor

Frcsty commented Mar 28, 2023

Apologies for the late response.

So the issue I'm facing is reloading an expansion (by reloading a plugin - shutting it down and reregistering) while the server is running results in PAPI not being able to reregister is, thus resulting in it erroring out - and since 90% of the plugins do not actually unregister the expansions themselves, this causes issues.

As for the reasoning behind this issue in the first place, sounds to be more of an issue with the implementing plugin, rather than this. It should also follow strict naming so
"Fixes an issue where the same expansion would be loaded twice, if the jar file used has a slightly different name."
seems kind of trivial, and is certainly an issue for the implementing plugin.

@Andre601
Copy link
Collaborator Author

The issue in your case would more be doing stuff that nobody encourages.

Reloading the server (or in your case individual plugins) is something no plugin dev would recommend to you. And the issues you describe are one reason why.

PlaceholderAPI doesn't monitor plugins that register expansions to then unload them when the plugin itself shuts down. This at most happens with expansions that defined a required plugin (See here).

The easiest way to fix this is to simply have the plugin unregister its expansion on disable to then register it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Target: Code This issue/PR is targeting the Code of PlaceholderAPI Type: Bugfix This fixes an existing bug in PlaceholderAPI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broken expansion loading

7 participants