fix: prevent cleanup command from deleting legacy IP log entries - #98
Merged
Conversation
📝 WalkthroughWalkthroughThis PR adds legacy-entry handling to IP log cleanup: entries with ChangesLegacy IP log timestamp initialization
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant CleanupIpLogCommand
participant IpLogRepository
participant Database
CleanupIpLogCommand->>IpLogRepository: countEntriesWithMissingTimestamp()
IpLogRepository->>Database: SELECT count WHERE tstamp = 0
Database-->>IpLogRepository: legacy count
IpLogRepository-->>CleanupIpLogCommand: legacy count
alt dry-run
CleanupIpLogCommand->>CleanupIpLogCommand: print "would initialize N"
CleanupIpLogCommand->>IpLogRepository: countEntriesLastSeenBefore(threshold)
IpLogRepository-->>CleanupIpLogCommand: delete count
CleanupIpLogCommand->>CleanupIpLogCommand: print "would delete N"
else execute
CleanupIpLogCommand->>IpLogRepository: initializeMissingTimestamps()
IpLogRepository->>Database: UPDATE tstamp = time() WHERE tstamp = 0
Database-->>IpLogRepository: updated count
IpLogRepository-->>CleanupIpLogCommand: updated count
CleanupIpLogCommand->>CleanupIpLogCommand: print "Initialized N"
CleanupIpLogCommand->>IpLogRepository: deleteEntriesLastSeenBefore(threshold)
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/Unit/Command/CleanupIpLogCommandTest.php (1)
113-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using
expects(self::once())instead ofmethod()for methods that must be called.Both
testDoesNotReportInitializationWhenNoLegacyEntriesExistandtestDryRunDoesNotReportInitializationWhenNoLegacyEntriesExistusemethod()to stub repository calls, which allows zero or more invocations. Since the command should always call these methods (in execute mode:initializeMissingTimestampsanddeleteEntriesLastSeenBefore; in dry-run mode:countEntriesWithMissingTimestampandcountEntriesLastSeenBefore), usingexpects(self::once())would catch regressions where the command accidentally skips these calls.♻️ Proposed refactor for stricter expectations
public function testDoesNotReportInitializationWhenNoLegacyEntriesExist(): void { $this->ipLogRepository - ->method('initializeMissingTimestamps') + ->expects(self::once()) + ->method('initializeMissingTimestamps') ->willReturn(0); $this->ipLogRepository - ->method('deleteEntriesLastSeenBefore') + ->expects(self::once()) + ->method('deleteEntriesLastSeenBefore') ->willReturn(5);public function testDryRunDoesNotReportInitializationWhenNoLegacyEntriesExist(): void { $this->ipLogRepository - ->method('countEntriesWithMissingTimestamp') + ->expects(self::once()) + ->method('countEntriesWithMissingTimestamp') ->willReturn(0); $this->ipLogRepository - ->method('countEntriesLastSeenBefore') + ->expects(self::once()) + ->method('countEntriesLastSeenBefore') ->willReturn(7);Also applies to: 156-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/Unit/Command/CleanupIpLogCommandTest.php` around lines 113 - 126, The repository stubs in CleanupIpLogCommandTest are too permissive for calls that must always happen. Update the relevant test methods, including testDoesNotReportInitializationWhenNoLegacyEntriesExist and testDryRunDoesNotReportInitializationWhenNoLegacyEntriesExist, to use expects(self::once()) on the IpLogRepository methods invoked by CleanupIpLogCommand so the test fails if the command skips initializeMissingTimestamps, deleteEntriesLastSeenBefore, countEntriesWithMissingTimestamp, or countEntriesLastSeenBefore.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Classes/Command/CleanupIpLogCommand.php`:
- Around line 62-71: The `CleanupIpLogCommand` dry-run messaging is outdated
because the command now reports both deletions and legacy timestamp
initializations. Update the `--dry-run` option description in
`CleanupIpLogCommand` so it accurately reflects the behavior shown in the
dry-run branch that calls `countEntriesWithMissingTimestamp()` and
`countEntriesLastSeenBefore()`.
---
Nitpick comments:
In `@Tests/Unit/Command/CleanupIpLogCommandTest.php`:
- Around line 113-126: The repository stubs in CleanupIpLogCommandTest are too
permissive for calls that must always happen. Update the relevant test methods,
including testDoesNotReportInitializationWhenNoLegacyEntriesExist and
testDryRunDoesNotReportInitializationWhenNoLegacyEntriesExist, to use
expects(self::once()) on the IpLogRepository methods invoked by
CleanupIpLogCommand so the test fails if the command skips
initializeMissingTimestamps, deleteEntriesLastSeenBefore,
countEntriesWithMissingTimestamp, or countEntriesLastSeenBefore.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 985f3c09-13c7-4dcf-9dc8-78383e0ec340
📒 Files selected for processing (5)
Classes/Command/CleanupIpLogCommand.phpClasses/Domain/Repository/IpLogRepository.phpREADME.mdTests/Unit/Command/CleanupIpLogCommandTest.phpTests/Unit/Domain/Repository/IpLogRepositoryTest.php
Comment on lines
+62
to
71
| $missing = $this->ipLogRepository->countEntriesWithMissingTimestamp(); | ||
| if ($missing > 0) { | ||
| $io->writeln(sprintf('%d legacy IP log entries without a last-seen timestamp would be initialized.', $missing)); | ||
| } | ||
|
|
||
| $count = $this->ipLogRepository->countEntriesLastSeenBefore($threshold); | ||
| $io->writeln(sprintf('%d IP log entries would be deleted (not seen for more than %d days).', $count, $days)); | ||
|
|
||
| return Command::SUCCESS; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the --dry-run option description to reflect new initialization reporting.
The option description at Line 45 still reads "Only report how many entries would be deleted", but the dry-run branch now also reports how many legacy entries would be initialized. This user-facing string should be updated for accuracy.
📝 Suggested fix
- ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Only report how many entries would be deleted');
+ ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Only report what would be changed without making modifications');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Classes/Command/CleanupIpLogCommand.php` around lines 62 - 71, The
`CleanupIpLogCommand` dry-run messaging is outdated because the command now
reports both deletions and legacy timestamp initializations. Update the
`--dry-run` option description in `CleanupIpLogCommand` so it accurately
reflects the behavior shown in the dry-run branch that calls
`countEntriesWithMissingTimestamp()` and `countEntriesLastSeenBefore()`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
typo3loginwarning:iplog:cleanup: entries created by extension versions ≤ 1.0.3 were inserted without a last-seen timestamp (tstamp = 0, the column default), so the first cleanup run deleted them all regardless of the configured retention period.--dry-runreports both the would-be-initialized and would-be-deleted counts without modifying anything.tstamp = 0rows entirely.Changes
Classes/Domain/Repository/IpLogRepository.php- NewinitializeMissingTimestamps()andcountEntriesWithMissingTimestamp()methods;tstamp > 0guard incountEntriesLastSeenBefore()anddeleteEntriesLastSeenBefore()Classes/Command/CleanupIpLogCommand.php- Initialize legacy entries before deleting; extended dry-run and run outputTests/Unit/Domain/Repository/IpLogRepositoryTest.php- Tests for the new methods and thetstamp > 0guardTests/Unit/Command/CleanupIpLogCommandTest.php- Tests for initialization, dry-run reporting, and suppressed output when no legacy entries existREADME.md- Document cleanup behavior for legacy entriesSummary by CodeRabbit
New Features
Bug Fixes
Documentation