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.

Sign up for GitHub

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

Jump to bottom

Remove PlayerPreprocessCommandEvent listener and clean up Commands #5966

Merged
merged 7 commits into from
Sep 26, 2023
Merged

Remove PlayerPreprocessCommandEvent listener and clean up Commands #5966

merged 7 commits into from
Sep 26, 2023

Conversation

sovdeeth
Copy link
Member

@sovdeeth sovdeeth commented Sep 3, 2023

Description

Skript currently listens to PPCE to check if a player is running a custom Skript command. If it determines they are, it cancels the event and runs the command. This, first of all, is a misuse of the Bukkit API. Secondly, it causes issues when listening with on command at priority HIGHEST or MONITOR. Thirdly, it prevents other plugins from being able to cancel or otherwise manipulate custom Skript commands.

Skript already registers commands properly via the Bukkit API, and has a working onCommand listener in the ScriptCommand class. This PR removes the PPCE listener and re-routes the commands through their proper onCommand listeners. It also renames the log player commands config option to log effect commands, as it no longer affects the logging of custom Skript commands, and hasn't for a while, I believe.

Finally, some other miscellaneous cleanup was done to Commands.java.

This PR needs more testing. I've done some and it all seems fine, but I'd be more comfortable if team members or Skript users could confirm that this doesn't break any existing behavior.

Target Minecraft Versions:
Requirements:
Related Issues: #5752 #5908

@sovdeeth sovdeeth added enhancement Feature request, an issue about something that could be improved, or a PR improving something. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. labels Sep 3, 2023
@AyhamAl-Ali
Copy link
Member

Can be sent to testing discord for more testing

@TheLimeGlass TheLimeGlass added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. 2.7 Targeting a 2.7.X version release breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels Sep 15, 2023
TheLimeGlass
TheLimeGlass approved these changes Sep 15, 2023
View reviewed changes
src/main/java/ch/njol/skript/command/Commands.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/command/Commands.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/command/Commands.java Outdated Show resolved Hide resolved
@APickledWalrus APickledWalrus removed the 2.7 Targeting a 2.7.X version release label Sep 15, 2023
@Moderocky Moderocky force-pushed the master branch 3 times, most recently from bd134d0 to 3f08853 Compare September 16, 2023 16:59
@AyhamAl-Ali AyhamAl-Ali changed the base branch from master to dev/feature September 16, 2023 17:17
@Moderocky Moderocky added patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. and removed patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. labels Sep 17, 2023
@Moderocky Moderocky added patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. and removed breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) labels Sep 17, 2023
APickledWalrus
APickledWalrus approved these changes Sep 26, 2023
View reviewed changes
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Looks good. I have a couple non-blocking suggestions.

src/main/java/ch/njol/skript/conditions/CondIsSkriptCommand.java
@@ -18,7 +18,7 @@
*/
package ch.njol.skript.conditions;

import static ch.njol.skript.command.Commands.skriptCommandExists;
import static ch.njol.skript.command.Commands.scriptCommandExists;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure there's a reason for this to be a static import considering it's used only once. We may want to consider changing it while we're here :)

src/main/java/ch/njol/skript/command/Commands.java
return handleEffectCommand(e.getPlayer(), e.getMessage());
}
});
Future<Boolean> f = Bukkit.getScheduler().callSyncMethod(Skript.getInstance(), () -> handleEffectCommand(event.getPlayer(), event.getMessage()));
Copy link
Member

Choose a reason for hiding this comment

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

This seems to replicate the behavior of the Task.callSync method. Might be worth using it here?

@Moderocky Moderocky merged commit d77ca01 into SkriptLang:dev/feature Sep 26, 2023
4 checks passed
Moderocky added a commit that referenced this pull request Sep 26, 2023
@sovdeeth @TheLimeGlass @Moderocky
Remove PlayerPreprocessCommandEvent listener and clean up Commands (#…
d1f73b8 
…5966)

* Remove PPCE listener and clean up Commands

* Apply suggestions from code review

Co-authored-by: LimeGlass <[email protected]>

* Update Commands.java

* we hate breaking changes

---------

Co-authored-by: LimeGlass <[email protected]>
Co-authored-by: Moderocky <[email protected]>
@sovdeeth sovdeeth deleted the investigate-proper-command-registration branch September 26, 2023 17:14
This was referenced Oct 15, 2023
Closed
Merged
@sovdeeth sovdeeth mentioned this pull request Oct 26, 2023
Closed
1 task
sovdeeth added a commit that referenced this pull request Dec 17, 2023
@APickledWalrus @UnderscoreTud
@sovdeeth @Moderocky @TheLimeGlass @AyhamAl-Ali @dependabot @NotSoDelayed @Spongecade @MihirKohli @3meraldK
Merge master into dev/feature (#6211)
09e8527 
* Fix ExprRandomNumber using a method from Java 17 (#6022)

* Fix changing remaining time of command cooldown (#6021)

Update ScriptCommand.java

Co-authored-by: Moderocky <[email protected]>

* Bump version to 2.7.1 (#5993)

Co-authored-by: Moderocky <[email protected]>

* fix 3 stray INSERT VERSIONs from 2.7.0 (#6027)

correct incorrect values

* Fix Documentation Actions on dev/patch (#6042)

* Tidy up parts of config class. (#6025)

* Add Release Model Document (#6041)

Add release model document

Co-authored-by: Ayham Al Ali <[email protected]>
Co-authored-by: Moderocky <[email protected]>

* (Cherry Pick) Fix cast throwing if existing variable for command storage exists (#5942) (#6026)

Fix cast throwing if existing variable for command storage exists (#5942)

* Fix cast throwing if existing variable for command storage exists

* Update src/main/java/ch/njol/skript/command/ScriptCommand.java



---------

Co-authored-by: LimeGlass <[email protected]>
Co-authored-by: Ayham Al Ali <[email protected]>

* (Cherry Pick) Fix NPE with invalid attributes and clean up ExprEntityAttribute (#5978) (#6023)

Fix NPE with invalid attributes and clean up ExprEntityAttribute (#5978)

* Avoid NPE and clean up class

* Update ExprEntityAttribute.java

* Update src/main/java/ch/njol/skript/expressions/ExprEntityAttribute.java



* Update src/main/java/ch/njol/skript/expressions/ExprEntityAttribute.java

---------

Co-authored-by: sovdee <[email protected]>
Co-authored-by: Ayham Al Ali <[email protected]>

* Fix multiple aliases sections not working (#6050)

* Fix error when unloading a script with multiple variables sections (#6047)

* Returns the old 2.6.4 duplicate variables section behaviour.

* Add an error but i don't know what it's for

* Add lots of brackets to keep walrus happy :}}}

* Add load tracker to prevent multiple loading.

* Prevent variable data wipe, fix another bug

* Support IDEs from the dark ages that don't know what a star is and think it orbits the earth or something

* add a test

---------

Co-authored-by: APickledWalrus <[email protected]>

* Bump actions/checkout from 3 to 4 (#6029)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

* ⚒ Disable Javadocs generation for nightly docs & improvements (#6059)

* Let's see if I am good at GH actions 🤞

* ops!

* Use proper docs template reference when possible

* Disable nightly javadocs generation with an option

Each javadoc is ~50mb, which was causing the big size of the docs! while each docs generation is ~2mb only

* Fix building

* Revert pull changes

They are not what fixed the issue, probably the old PRs aren't syncing for some reason

* Update build.gradle

---------

Co-authored-by: Moderocky <[email protected]>

* Change the target branch of dependabot (#6063)

Update dependabot.yml

* ⚒ Fix stop all sounds NPE (#6067)

* Bump actions/checkout from 3 to 4 (#6069)

Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4.
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@v3...v4)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: LimeGlass <[email protected]>

* Bump org.gradle.toolchains.foojay-resolver-convention from 0.5.0 to 0.7.0 (#6070)

Bump org.gradle.toolchains.foojay-resolver-convention

Bumps org.gradle.toolchains.foojay-resolver-convention from 0.5.0 to 0.7.0.

---
updated-dependencies:
- dependency-name: org.gradle.toolchains.foojay-resolver-convention
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: LimeGlass <[email protected]>

* Bump org.easymock:easymock from 5.1.0 to 5.2.0 (#6071)

Bumps [org.easymock:easymock](https://github.com/easymock/easymock) from 5.1.0 to 5.2.0.
- [Release notes](https://github.com/easymock/easymock/releases)
- [Changelog](https://github.com/easymock/easymock/blob/master/ReleaseNotes.md)
- [Commits](easymock/easymock@easymock-5.1.0...easymock-5.2.0)

---
updated-dependencies:
- dependency-name: org.easymock:easymock
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: LimeGlass <[email protected]>

* Bump io.papermc.paper:paper-api from 1.20.1-R0.1-SNAPSHOT to 1.20.2-R0.1-SNAPSHOT (#6072)

* Bump io.papermc.paper:paper-api

Bumps io.papermc.paper:paper-api from 1.20.1-R0.1-SNAPSHOT to 1.20.2-R0.1-SNAPSHOT.

---
updated-dependencies:
- dependency-name: io.papermc.paper:paper-api
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>

* Apply 1.20.2 to the test runner

* Deprecate org.bukkit.util.Consumer usage in EntityData

* Adapt against the Java Consumer instead of Bukkit's

* Resolve existing method deprecation

* Adapt against the Java Consumer instead of Bukkit's

* Update developer note

* Result in reflection for Bukkit Consumer

* Resolve ThrownPotion Consumer

* Result in reflection for Bukkit Consumer

* Pretty else if

* Add common reflective spawn method.

* Use common spawn method in potion class.

* Remove old suppression!

* Whoops I forgot about the consumer

* Don't need reflection import anymore :)

* Thrown potion class

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: LimeGlass <[email protected]>
Co-authored-by: TheLimeGlass <[email protected]>
Co-authored-by: Moderocky <[email protected]>

* Pull request template defaults (#5665)

Update pull_request_template.md

* Fix EvtPlayerChunkEnter Comparison & Cleanup (#5965)

Initial

(cherry picked from commit 389c002)

Co-authored-by: Moderocky <[email protected]>

* Fixes EffSecSpawn not properly handling local variables created within the section (#6033)

Communicate local variables between consumer calls

thanks pickle

Co-authored-by: Moderocky <[email protected]>

* Remove PlayerPreprocessCommandEvent listener and clean up Commands (#5966)

* Remove PPCE listener and clean up Commands

* Apply suggestions from code review

Co-authored-by: LimeGlass <[email protected]>

* Update Commands.java

* we hate breaking changes

---------

Co-authored-by: LimeGlass <[email protected]>
Co-authored-by: Moderocky <[email protected]>

* Clean up vector classes and fix a few bugs.

* More improvements

* Apply suggestions from code review

Co-authored-by: Ayham Al Ali <[email protected]>

* Budget Expansion

* Fix Logging Issues In ExpressionEntryData (#6081)

Fix duplicate logging

* Prepare For Release 2.7.1 (#6082)

* Update Minecraft wiki links to new domain (#6078)

* ⚒ Fix fake player count paper check error (#6090)

* Fix Command Help (#6080)

Fix issues and cleanup CommandHelp class

Co-authored-by: Moderocky <[email protected]>

* Bump net.kyori:adventure-text-serializer-bungeecord from 4.3.0 to 4.3.1 (#6084)

Bumps [net.kyori:adventure-text-serializer-bungeecord](https://github.com/KyoriPowered/adventure-platform) from 4.3.0 to 4.3.1.
- [Release notes](https://github.com/KyoriPowered/adventure-platform/releases)
- [Commits](KyoriPowered/adventure-platform@v4.3.0...v4.3.1)

---
updated-dependencies:
- dependency-name: net.kyori:adventure-text-serializer-bungeecord
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ayham Al Ali <[email protected]>

* Fix unloading/reloading a directory in the scripts effect (#6106)

* Moved unloading to a common method.

* Accidental double space 😬

* Force UTF-8 encoding for Gradle daemon (#6103)

Co-authored-by: Moderocky <[email protected]>

* Corrected Javadocs name, title (#6038)

* Javadoc Title,Name

* Update .gitignore

Co-authored-by: LimeGlass <[email protected]>

---------

Co-authored-by: Ayham Al Ali <[email protected]>
Co-authored-by: LimeGlass <[email protected]>
Co-authored-by: Moderocky <[email protected]>

* Rebase JUnit references fix for dev/patch (#6057)

* Fix options issue in functions (#6121)

* Fix command permission messages (2.7.1 issue) (#6126)

re-add permission handling during PPCE to get around spigot behavior.

* Fix stack overflow when stringifying block inventories. (#6117)

* Fix stack overflow when stringifying block inventories.

* Blanket catch until a better solution is found.

* Fix comparison of cyclical types (specifically comparing times) (#6128)

* Add cyclical type helper.

* Make time cyclical.

* Add special comparison for cyclical types.

* Add some tests.

* Fix floating point rounding error in loop N times (#6132)

Fix floating point error in loop X times.

* Fix Sorted List Expression (#6102)

* Fix ExprSortedList

* Update src/main/java/ch/njol/skript/expressions/ExprSortedList.java

Co-authored-by: LimeGlass <[email protected]>

* Fix test

Co-authored-by: Moderocky <[email protected]>

---------

Co-authored-by: LimeGlass <[email protected]>
Co-authored-by: Moderocky <[email protected]>

* Fix colour codes being reset in reload message. (#6150)

Fix colour codes being reset.

* Fix ExprDurability's Changer (#6154)

* Fix ExprDurability's changer

* Change method name.

* Add simple test.

---------

Co-authored-by: Moderocky <[email protected]>

* Catch the exception when pushing entity by non finite vector (#5765)

* Fix issues with ExprDrops (#6130)

* refactor ExprDrops, fix bugs, add test

fixed setting drops to multiple items/experience values at once
fixed null values being left in drops after removing items
maintained behavior but behavior needs a big update

* small cleanup

* Fix JUnit test location

* import shenanigans

---------

Co-authored-by: Moderocky <[email protected]>

* Prepare For Release (2.7.2) (#6166)

* Prevent InventoryHolder -> X chaining (#6171)

* Prevent InventoryHolder -> X chaining

* Improve Location Comparison (#6205)

* Allow asynchronous SkriptEvent#check execution (#6201)

* Fix ExprSets conflicting (#6123)

* Prepare for Release (2.7.3) (#6208)

* Further corrections

* Fix NPE issue with drops in 1.20.2

* Update StructFunction.java

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: _tud <[email protected]>
Co-authored-by: sovdee <[email protected]>
Co-authored-by: Moderocky <[email protected]>
Co-authored-by: LimeGlass <[email protected]>
Co-authored-by: Ayham Al Ali <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: TheLimeGlass <[email protected]>
Co-authored-by: DelayedGaming <[email protected]>
Co-authored-by: Spongecade <[email protected]>
Co-authored-by: MihirKohli <[email protected]>
Co-authored-by: _tud <[email protected]>
Co-authored-by: 3meraldK <[email protected]>
@sovdeeth sovdeeth mentioned this pull request Apr 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@APickledWalrus APickledWalrus APickledWalrus approved these changes

@TheLimeGlass TheLimeGlass TheLimeGlass approved these changes

@Moderocky Moderocky Moderocky approved these changes

@Pikachu920 Pikachu920 Awaiting requested review from Pikachu920

Assignees
No one assigned
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sovdeeth @AyhamAl-Ali @Moderocky @TheLimeGlass @APickledWalrus