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

Toggle off certain features & fix various bugs #128

Merged
merged 26 commits into from
May 14, 2023
Merged

Toggle off certain features & fix various bugs #128

merged 26 commits into from
May 14, 2023

Conversation

FahZohh
Copy link
Contributor

@FahZohh FahZohh commented Apr 24, 2023

Please make sure your pull request complies with these guidelines:

    • Use same formatting
    • Changes must have been tested on PMMP.
    • Unless it is a minor code modification, you must use an IDE.
    • Have a detailed title.

What does the PR change?

Testing Environment

  • PHP:
  • PMMP:
  • OS:

@Aericio
Copy link
Collaborator

Aericio commented Apr 24, 2023

  • Why did you remove all the subcommands for /f claim and /f unclaim?
  • Why did you remove /f enemy?
  • You should be loading the virions using devirion plugin, NOT adding the libs directly.

@FahZohh
Copy link
Contributor Author

FahZohh commented Apr 24, 2023

  • Why did you remove all the subcommands for /f claim and /f unclaim?
  • Why did you remove /f enemy?
  • You should be loading the virions using devirion plugin, NOT adding the libs directly.

when we do the /f claim circle 999999999999999999999999999999999 or other it crashes the server, I removed the f enemy but I think I'm going to put it back and offer people to activate or deactivate it.

@Thunder33345
Copy link
Collaborator

we cant even cherry pick the good stuff out because it's all in one single commit

@FahZohh
Copy link
Contributor Author

FahZohh commented Apr 24, 2023

we cant even cherry pick the good stuff out because it's all in one single commit

What do you want to take? You want me to do multiple commits? One for subcommands and the others

@FahZohh FahZohh changed the title Fix : remove all subcommands for /f claim and /f unclaim, remove /f enemy, add limit for the /f top and add config for the /f fly. Fix : add config for /f enemy, /f fly , add limit for /f top, /f claim square, /f claim circle (with limit config) Apr 24, 2023
@FahZohh
Copy link
Contributor Author

FahZohh commented Apr 24, 2023

I did everything as you advised me if you have any suggestions or advice I will listen to them

@Aericio Aericio self-requested a review April 25, 2023 01:16
@Aericio Aericio linked an issue Apr 25, 2023 that may be closed by this pull request
@Aericio Aericio added this to the 2.0.4 milestone Apr 25, 2023
@Aericio Aericio linked an issue Apr 25, 2023 that may be closed by this pull request
@Aericio Aericio changed the title Fix : add config for /f enemy, /f fly , add limit for /f top, /f claim square, /f claim circle (with limit config) Toggle off certain features & fix various bugs Apr 25, 2023
@Aericio
Copy link
Collaborator

Aericio commented Apr 25, 2023

I will review this next month.

FahZohh and others added 2 commits April 25, 2023 11:46
Co-authored-by: Aericio <16523741+Aericio@users.noreply.github.com>
… limit and set true to default for the new config
@FahZohh
Copy link
Contributor Author

FahZohh commented Apr 26, 2023

I put everything you advise, thank you very much

@Aericio
Copy link
Collaborator

Aericio commented May 13, 2023

We should just add command blacklisting instead, probably.

@Aericio Aericio requested a review from DaPigGuy May 13, 2023 13:03
@Aericio Aericio linked an issue May 13, 2023 that may be closed by this pull request
@FahZohh
Copy link
Contributor Author

FahZohh commented May 13, 2023

Having set limit for the /f top "PHP_INT_MAX" does not fix the main bug which is /f top power or member 999999999999999 and -999999999999999

@dadodasyra
Copy link
Contributor

Why this PR deals with so many things..

By the way, limiting to PHP_INT_MAX will probably not suffice for commands that require complex calculations or database calls. As it could lead to hidden performance issues.

@FahZohh
Copy link
Contributor Author

FahZohh commented May 13, 2023

All commands that have an int can cause a crash, they should all be fixed at the same time

@Aericio
Copy link
Collaborator

Aericio commented May 14, 2023

@dadodasyra thanks for the feedback.

I'll most likely cherrypick the commits from this PR instead of merging.

About PHP_INT_MAX, should we just set the maximum to some reasonable limit instead then? like 100000.

@FahZohh Agreed. Maybe a helper function to validate numbers. Not sure what the best way to handle this is.

@FahZohh
Copy link
Contributor Author

FahZohh commented May 14, 2023

so we need to put a limit of 100000 for all commands that contain an int, but I have a question that won't do like last time or an extremely large number would probably result in an int overflow thus causing it to become a float ?

@Aericio
Copy link
Collaborator

Aericio commented May 14, 2023

Having set limit for the /f top "PHP_INT_MAX" does not fix the main bug which is /f top power or member 999999999999999 and -999999999999999

Oh, I see why this is still the case.

foreach (array_slice($factions, $page * self::PAGE_LENGTH, self::PAGE_LENGTH) as $rank => $faction)

Page being multiplied by 10 here, so it'll just turn back into a float here.

@Aericio
Copy link
Collaborator

Aericio commented May 14, 2023

This should be a better solution. We set the max as the number of possible factions, instead of PHP_INT_MAX.

@Aericio Aericio removed the request for review from DaPigGuy May 14, 2023 10:43
@Aericio
Copy link
Collaborator

Aericio commented May 14, 2023

ok, seems to be working. don't feel like cherrypicking so weee

@Aericio Aericio merged commit 85d6e24 into DaPigGuy:master May 14, 2023
@Aericio Aericio self-assigned this May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants