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

Fix: Time-based tracking fix for ASP #51

Merged
merged 3 commits into from
Jun 15, 2023

Conversation

darth3pio
Copy link

These changes fix the error that gets thrown when attempting to view player details in Private Stats Admin.

Adding missing 'timestamp' column in 'player_unlock' table to db schema
Restore $time to "player_unlock" insert statement
Fix for TimeSpan issue in PHP 7.4.x
@leojonathanoh
Copy link
Member

Hey @darth3pio, thanks much for the contribution 😄 Never really viewed player details in Private Stats Admin, i suppose the exception must be due to the syntax error in switch ($format[$o - 1]) in src/ASP/system/framework/TimeSpan.php?

I guess this PR could be split into two, one with a fix for the syntax error, and one with a feature adding time-based tracking for player unlocks which adds a new column in the player_unlocks table. I hope i make sense?

@darth3pio
Copy link
Author

darth3pio commented Jun 14, 2023

If you look at older commits from Wilson212, The feature was already there, it was one of the community members of the BF2Statistics forums that suggested removing $time from the selectunlock.php function as they couldn't figure out the issue they were looking at. It was two things a) Unlocks weren't unlocking and b) Viewing player pages in Private Stats Admin would throw an error when the time had to be formatted.

It's tricky to separate the two as they both rely on each other at least if you want to run it on a newer PHP version.

Viewing a player page (PlayerModel.php) still heavily queried $time so without commenting out everything it wouldn't actually fix that issue.

I narrowed it down to

  1. The reason unlocks hadn't been unlocking was because the timespan column didn't exist so I updated the schema.sql to add it in when the db was generated. (Removing $time from selectunlock.php was a bandaid fix)
  2. This also required that selectunlock.php actually generate a timestamp to enter into the database when you select an unlock in game.
  3. Finally, when the PlayerModel.php queried the player_unlock table for a valid timestamp it would then format the time using TimeSpan.php, something which was throwing an error in anything newer than PHP 7.3.x

@leojonathanoh leojonathanoh self-assigned this Jun 14, 2023
@leojonathanoh leojonathanoh added this to the 3.2.0 milestone Jun 14, 2023
@leojonathanoh
Copy link
Member

Thanks for the detailed explanation. Just spun up a development environment, i see the exception is:

Error Description

SQLSTATE[42S22]: Column not found: 1054 Unknown column 'timestamp' in 'field list'
Error Details

    Type: PDOException
    Code: 42S22
    File: /src/ASP/frontend/modules/players/models/PlayerModel.php
    Line: 928

Stack Trace

    /frontend/modules/players/models/PlayerModel.php @ line 928:
        PDO->query('SELECT `unlock_id`, `timestamp` FROM `player_unlock` WHERE `player_id`=2900000 ORDER BY `unlock_id` ASC')
    /frontend/modules/players/Players.php @ line 126:
        PlayerModel->attachUnlockData(2900000, System\View())
    (unknown file) @ line 0:
        Players->view(2900000)
    /system/System.php @ line 336:
        ReflectionMethod->invokeArgs(Players(), array(1))
    /system/System.php @ line 260:
        System::LoadModule('players', 'view', array(1))
    /system/System.php @ line 54:
        System::HandleAdminRequest()
    /index.php @ line 47:
        System::Run()

which like you explained is because of a missing column in player_unlock table in the schema.sql. So I guess this PR is correct that this should be a fix.

Don't see a point opening a separate PR just for fixing the deprecated syntax $format{$o - 1} for php >= 7.3.

But overall, spinning up the stack from scratch on this PR, the player view now works properly in stats admin.

Just one little change. Could you prefix the PR title with Fix: ..., just to keep with conventional commits. Other than that it is good to go.

@darth3pio darth3pio changed the title Time-based tracking fix for ASP Fix: Time-based tracking fix for ASP Jun 15, 2023
@leojonathanoh
Copy link
Member

lgtm. Thanks for your contribution! 😄

@leojonathanoh leojonathanoh merged commit e3bc26b into startersclan:master Jun 15, 2023
@darth3pio darth3pio deleted the patch-4 branch June 15, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants