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

[ticket/15985] List ban duration in logs #5691

Open
wants to merge 1 commit into
base: 3.2.x
Choose a base branch
from

Conversation

stevendegroote
Copy link
Contributor

@stevendegroote stevendegroote commented Sep 17, 2019

Log the length of a ban, so moderators can check afterwards how long previous
bans were.

PHPBB3-15985

Checklist:

  • Correct branch: master for new features; 3.3.x & 3.2.x for fixes
  • Tests pass
  • Code follows coding guidelines: master, 3.3.x and 3.2.x
  • Commit follows commit message format

Tracker ticket (set the ticket ID to your ticket ID):

https://tracker.phpbb.com/browse/PHPBB3-15985

Log the length of a ban, so moderators can check afterwards how long previous
bans were.

PHPBB3-15985
@marc1706
Copy link
Member

Hm, I don't really like the approach you're taking. You're adding handling for a very specific type of log to the log class whereas that class should only take care about adding and displaying logs, not about handling specifics inside those logs. Also, I think you're current approach will cause at least PHP warnings with displaying bans that were created before this change as $log_data_ary[2] and $log_data_ary[3] will not be set.

@stevendegroote
Copy link
Contributor Author

Thanks @marc1706 for looking into it.

First of all, there are no warnings in case the arguments are not set. On lines 675-700 in log.php, there is code that completes missing arguments for the printf method, so that's covered (I was frankly surprised we have such code, but anyway)

I think this change is very much needed. I'm doing workarounds for years on this matter, and there's no true possibility to do this via an extension. To me, it's a vital functionality that's missing for moderators.

I'd by happy to re-implement though, but what do you propose then?

@stevendegroote
Copy link
Contributor Author

stevendegroote commented Jul 24, 2023

@marc1706 Old, but not forgotten PR....

So, you'd like to see the translation of arg2 and arg3 to "1 month until 24/08/2023" to not happen in the log class?
I could of course move that to a function in functions_user.php

As for the PHP warnings comment, I can double check but think the 20 lines above my change in log.php take care of that. For older logs you would of course see "Banned user dummy () for reason 'silliness'" (so nothing between the brackets). Not sure how to avoid that unless I move the bracket addition into the translation mapping as described above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants