Skip to content

Conversation

@mdcfe
Copy link
Member

@mdcfe mdcfe commented Jun 14, 2018

This PR cherry-picks commits from #1944. The differences between this PR and #1944 are:

To do:

  • Format placeholders in translated messages sent to the log
  • Escape placeholders in mail/AFK messages
  • Replace placeholders in mail/AFK formats
  • Consider supporting "recipient placeholders" in formats, which are left unescaped and only parsed when received by a user?
  • Escape unformatted %s in chat format
  • Any other broadcast messages

Credits go to @Banbeucmas for the original PR.

Closes #1945, closes #1944, closes #1959, yada yada

@mdcfe mdcfe added the type: enhancement Features and feature requests. label Jun 14, 2018
@mdcfe mdcfe added this to the 2.16.0 milestone Jun 14, 2018
@mdcfe mdcfe requested a review from drtshock June 14, 2018 12:28
@Banbeucmas
Copy link
Contributor

Doesn't include replacing {...} with %...%, as that would be confusing for users when old Essentials placeholders break

While I understand the reason, but it would seem weird if the format contain {} and one contain %% in the same config file on the other hand.

@mdcfe
Copy link
Member Author

mdcfe commented Jun 16, 2018

@Banbeucmas It's more clear whether a placeholder comes from EssentialsX or PlaceholderAPI if they have different characters surrounding them.

@Banbeucmas
Copy link
Contributor

Banbeucmas commented Jun 17, 2018

Essentials placeholder already use uppercase character for most of the time , I think it is safe to just use {} to replace %%.
But again, we can ask whenever an user prefer %% and {} seperately or not - a poll?, I predict there will be an issue around this after the merge sooner or later. (It’s doesn’t sync with the text, so that’s what made it strange, it is reasonable to ask that)

@mdcfe mdcfe requested a review from kashike June 17, 2018 14:54
@mdcfe
Copy link
Member Author

mdcfe commented Jun 17, 2018

@Banbeucmas The Essentials docs and config usually include capitalised tokens, but they are usually case-insensitive when replaced as far as I remember.

Regardless, it would be simpler for PAPI placeholders to work the same in EssentialsX as they do in every single other plugin that supports PAPI (just %%), as users will be able to just drop in the same %% placeholders that they're used to in other plugins. People also won't complain that {} doesn't work outside EssentialsX, reducing support noise.

@Banbeucmas
Copy link
Contributor

Banbeucmas commented Jun 18, 2018

I guess that will be how it's goes then. Up to you.

@extendedclip
Copy link

extendedclip commented Jun 18, 2018

Using %<placeholder>% in chat format will cause an IllegalFormatException to be thrown if the placeholder is not parsed or escaped before the server handles formatting the chat message.

@mdcfe
Copy link
Member Author

mdcfe commented Jun 18, 2018

@extendedclip Thanks for the heads-up. I've added this to the to-do.

@Ichbinjoe Ichbinjoe added the help wanted Issues that need further investigation. label Apr 17, 2019
@mdcfe mdcfe removed this from the 2.17.0 milestone Apr 18, 2019
@mdcfe mdcfe self-assigned this Apr 18, 2019
@mdcfe mdcfe added this to the 2.18.0 milestone Jun 26, 2019
@mdcfe mdcfe removed this from the 2.18.0 milestone Apr 14, 2020
@mdcfe
Copy link
Member Author

mdcfe commented Apr 19, 2020

There are issues with the approach taken in this PR, and for that reason I'm going to close this. #2502 is a better base for this to be implemented on.

@pop4959 pop4959 closed this May 27, 2020
@TheKrafter TheKrafter mentioned this pull request Nov 24, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Issues that need further investigation. type: enhancement Features and feature requests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PlaceholderAPI support

7 participants