Skip to content

[v14] Phpstan fixes #1021

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

Merged
merged 1 commit into from
May 6, 2025
Merged

[v14] Phpstan fixes #1021

merged 1 commit into from
May 6, 2025

Conversation

erikn69
Copy link
Contributor

@erikn69 erikn69 commented May 6, 2025

https://github.com/owen-it/laravel-auditing/pull/933/files
In the old version (->get()->slice($threshold)->pluck('id')) Memory performance was failing, since with get() all the data was brought as eloquent models

Now (->pluck($keyName)->all()) pluck/all is done directly, which would only use an int array

whereIn was replaced by whereIntegerInRaw(avoid too many Placeholders issue)

The same solution works on older Laravel versions, if merged it would also be released in v13: https://github.com/owen-it/laravel-auditing/actions/runs/14862464393/job/41730649233

UPDATE: whereIntegerInRaw would cause problems when the key is a string, like UUID?

@erikn69
Copy link
Contributor Author

erikn69 commented May 6, 2025

whereIntegerInRaw would cause problems when the key is a string, like UUID?

@willpower232 ??

@willpower232
Copy link
Contributor

True, you would have to edit the ID column of the audits table to actually make that a problem though.

Depends how much of a risk you think that is, that someone would both enable pruning AND change the type of the ID column.

@erikn69
Copy link
Contributor Author

erikn69 commented May 6, 2025

would something like this work?

->whereRaw($keyName ." IN ('".implode("', '", $forRemoval)."')")

I think in MariaDb, it would cover int and string, but I don't know about other DB managers

@willpower232
Copy link
Contributor

yeah, it can't run out of placeholders if there aren't any 👌

@erikn69
Copy link
Contributor Author

erikn69 commented May 6, 2025

https://github.com/laravel/framework/blob/94c26967f8f7eebd61af49a90f1ee49b1bb6758b/src/Illuminate/Database/Query/Grammars/Grammar.php#L389-L393

I see that the same thing is used in all the grammars, but the question was if the field is int, when making it a string "My_INT", does it work the same in any DB manager? PostGreSQL for example

@erikn69 erikn69 force-pushed the erikn69-patch-3 branch from 5a2e9f5 to 0d0c080 Compare May 6, 2025 21:39
@erikn69 erikn69 force-pushed the erikn69-patch-3 branch from 0d0c080 to 8050da0 Compare May 6, 2025 21:42
@erikn69
Copy link
Contributor Author

erikn69 commented May 6, 2025

Making some comparisons it seems to me that the current leftJoinSub has better performance, so in this PR I will only leave the phpstan fixes.

Also #1019 (comment)

btw: we deployed today to the live box:
The issue mentioned in this ticket did not happen

In case it is used again in the future, the code that was there is:

public function prune(Auditable $model): bool
{
    if (($threshold = $model->getAuditThreshold()) > 0) {
        $auditClass = get_class($model->audits()->getModel());
        $keyName = (new $auditClass)->getKeyName();
        $forRemoval = array_slice(
            $model->audits()->latest()->pluck($keyName)->all(),
            $threshold
        );
        
        if (count($forRemoval)) {
            if (is_string($forRemoval[0])) {
                $forRemoval = "'" . implode("', '", $forRemoval) . "'";
            } else {
                $forRemoval = implode(', ', $forRemoval);
            }

            return $model->audits()
                ->whereRaw($keyName ." in ($forRemoval)")
                ->delete() > 0;
        }
    }

    return false;
}

@erikn69 erikn69 changed the title Fix audits prune(revert and fix old version), and phpstan fixes [v14] Phpstan fixes May 6, 2025
@erikn69 erikn69 merged commit 177715e into master May 6, 2025
13 checks passed
@erikn69 erikn69 deleted the erikn69-patch-3 branch May 6, 2025 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants