Skip to content

fix: prevent cleanup command from deleting legacy IP log entries #98

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.

Sign up for GitHub

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
konradmichalik merged 2 commits into from
Jul 8, 2026
Merged

fix: prevent cleanup command from deleting legacy IP log entries #98

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d5adcb
fix: initialize legacy IP log entries without last-seen timestamp dur…
konradmichalik Jul 8, 2026
46a7484
docs: document cleanup handling of legacy IP log entries
konradmichalik Jul 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Classes/Command/CleanupIpLogCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,22 @@ protected function execute(InputInterface $input, OutputInterface $output): int
$threshold = time() - $days * self::SECONDS_PER_DAY;

if (true === $input->getOption('dry-run')) {
$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;
}
Comment on lines +62 to 71

@coderabbitai coderabbitai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 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()`.


$initialized = $this->ipLogRepository->initializeMissingTimestamps();
if ($initialized > 0) {
$io->writeln(sprintf('Initialized %d legacy IP log entries without a last-seen timestamp.', $initialized));
}

$deleted = $this->ipLogRepository->deleteEntriesLastSeenBefore($threshold);
$io->writeln(sprintf('Deleted %d IP log entries (not seen for more than %d days).', $deleted, $days));

Expand Down
39 changes: 39 additions & 0 deletions Classes/Domain/Repository/IpLogRepository.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ public function registerIdentifier(string $identifierHash): bool
}

/**
* Entries with tstamp = 0 stem from extension versions that did not track the
* last sighting. Their real age is unknown, so they are excluded here and must
* be initialized via initializeMissingTimestamps() before any cleanup.
*
* @throws Exception
*/
public function countEntriesLastSeenBefore(int $timestamp): int
Expand All @@ -76,6 +80,7 @@ public function countEntriesLastSeenBefore(int $timestamp): int
->from(self::TABLE_NAME)
->where(
$queryBuilder->expr()->lt('tstamp', $queryBuilder->createNamedParameter($timestamp, Connection::PARAM_INT)),
$queryBuilder->expr()->gt('tstamp', $queryBuilder->createNamedParameter(0, Connection::PARAM_INT)),
)
->executeQuery()->fetchOne();
}
Expand All @@ -88,7 +93,41 @@ public function deleteEntriesLastSeenBefore(int $timestamp): int
->delete(self::TABLE_NAME)
->where(
$queryBuilder->expr()->lt('tstamp', $queryBuilder->createNamedParameter($timestamp, Connection::PARAM_INT)),
$queryBuilder->expr()->gt('tstamp', $queryBuilder->createNamedParameter(0, Connection::PARAM_INT)),
)
->executeStatement();
}

/**
* @throws Exception
*/
public function countEntriesWithMissingTimestamp(): int
{
$queryBuilder = $this->connectionPool->getQueryBuilderForTable(self::TABLE_NAME);

return (int) $queryBuilder
->count('uid')
->from(self::TABLE_NAME)
->where(
$queryBuilder->expr()->eq('tstamp', $queryBuilder->createNamedParameter(0, Connection::PARAM_INT)),
)
->executeQuery()->fetchOne();
}

/**
* Backfills the last-seen timestamp of legacy entries with the current time,
* granting them a full retention period before they become cleanup candidates.
*
* @throws Exception
*/
public function initializeMissingTimestamps(): int
{
$connection = $this->connectionPool->getConnectionForTable(self::TABLE_NAME);

return $connection->update(
self::TABLE_NAME,
['tstamp' => time()],
['tstamp' => 0],
);
}
}
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@ Use `--dry-run` to only report how many entries would be deleted. The command is
> [!NOTE]
> After an entry has been deleted, the next login from that IP address triggers a new-IP notification again — that is the intended effect of a retention period.

Entries created by older extension versions (1.0.3 and below) may lack a last-seen timestamp. The cleanup command initializes such legacy entries with the current time instead of deleting them, so they are granted a full retention period before becoming cleanup candidates — otherwise every already-known IP would be reported as new again after the first cleanup run.

#### Geolocation

If `Fetch Geolocation` is enabled, the extension will use the [ip-api.com](https://ip-api.com/) service to fetch geolocation information for the IP address. Only public IP addresses will be looked up to respect privacy.
Expand Down
78 changes: 78 additions & 0 deletions Tests/Unit/Command/CleanupIpLogCommandTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,84 @@ public function testDryRunOnlyCountsEntries(): void
self::assertStringContainsString('7 IP log entries would be deleted', $this->commandTester->getDisplay());
}

public function testInitializesLegacyEntriesBeforeDeleting(): void
{
$this->ipLogRepository
->expects(self::once())
->method('initializeMissingTimestamps')
->willReturn(3);

$this->ipLogRepository
->expects(self::once())
->method('deleteEntriesLastSeenBefore')
->willReturn(5);

$exitCode = $this->commandTester->execute([]);

self::assertSame(Command::SUCCESS, $exitCode);
$display = $this->commandTester->getDisplay();
self::assertStringContainsString('Initialized 3 legacy IP log entries', $display);
self::assertStringContainsString('Deleted 5 IP log entries', $display);
}

public function testDoesNotReportInitializationWhenNoLegacyEntriesExist(): void
{
$this->ipLogRepository
->method('initializeMissingTimestamps')
->willReturn(0);

$this->ipLogRepository
->method('deleteEntriesLastSeenBefore')
->willReturn(5);

$this->commandTester->execute([]);

self::assertStringNotContainsString('Initialized', $this->commandTester->getDisplay());
}

public function testDryRunReportsLegacyEntriesWithoutModifying(): void
{
$this->ipLogRepository
->expects(self::once())
->method('countEntriesWithMissingTimestamp')
->willReturn(4);

$this->ipLogRepository
->expects(self::once())
->method('countEntriesLastSeenBefore')
->willReturn(7);

$this->ipLogRepository
->expects(self::never())
->method('initializeMissingTimestamps');

$this->ipLogRepository
->expects(self::never())
->method('deleteEntriesLastSeenBefore');

$exitCode = $this->commandTester->execute(['--dry-run' => true]);

self::assertSame(Command::SUCCESS, $exitCode);
$display = $this->commandTester->getDisplay();
self::assertStringContainsString('4 legacy IP log entries without a last-seen timestamp would be initialized', $display);
self::assertStringContainsString('7 IP log entries would be deleted', $display);
}

public function testDryRunDoesNotReportInitializationWhenNoLegacyEntriesExist(): void
{
$this->ipLogRepository
->method('countEntriesWithMissingTimestamp')
->willReturn(0);

$this->ipLogRepository
->method('countEntriesLastSeenBefore')
->willReturn(7);

$this->commandTester->execute(['--dry-run' => true]);

self::assertStringNotContainsString('would be initialized', $this->commandTester->getDisplay());
}

public function testFailsForNonPositiveDays(): void
{
$this->ipLogRepository
Expand Down
80 changes: 64 additions & 16 deletions Tests/Unit/Domain/Repository/IpLogRepositoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,20 +114,23 @@ public function testRegisterIdentifierReturnsFalseWhenConcurrentLoginInsertedFir
self::assertFalse($this->subject->registerIdentifier($identifierHash));
}

public function testCountEntriesLastSeenBefore(): void
public function testCountEntriesLastSeenBeforeExcludesEntriesWithoutTimestamp(): void
{
$this->queryBuilder->expects(self::once())
->method('count')
->with('uid')
->willReturnSelf();
$this->queryBuilder->method('from')->willReturnSelf();
$this->queryBuilder->expects(self::once())
->method('createNamedParameter')
->with(12345, Connection::PARAM_INT)
->willReturn(':tstamp');
$this->expressionBuilder->method('lt')
->with('tstamp', ':tstamp')
->willReturn('tstamp < :tstamp');
$this->queryBuilder->method('createNamedParameter')
->willReturnCallback(static fn (int $value): string => ':param'.$value);
$this->expressionBuilder->expects(self::once())
->method('lt')
->with('tstamp', ':param12345')
->willReturn('tstamp < :param12345');
$this->expressionBuilder->expects(self::once())
->method('gt')
->with('tstamp', ':param0')
->willReturn('tstamp > :param0');
$this->queryBuilder->method('where')->willReturnSelf();

$result = $this->createMock(Result::class);
Expand All @@ -142,19 +145,22 @@ public function testCountEntriesLastSeenBefore(): void
self::assertSame(3, $this->subject->countEntriesLastSeenBefore(12345));
}

public function testDeleteEntriesLastSeenBefore(): void
public function testDeleteEntriesLastSeenBeforeExcludesEntriesWithoutTimestamp(): void
{
$this->queryBuilder->expects(self::once())
->method('delete')
->with('tx_typo3loginwarning_iplog')
->willReturnSelf();
$this->queryBuilder->expects(self::once())
->method('createNamedParameter')
->with(12345, Connection::PARAM_INT)
->willReturn(':tstamp');
$this->expressionBuilder->method('lt')
->with('tstamp', ':tstamp')
->willReturn('tstamp < :tstamp');
$this->queryBuilder->method('createNamedParameter')
->willReturnCallback(static fn (int $value): string => ':param'.$value);
$this->expressionBuilder->expects(self::once())
->method('lt')
->with('tstamp', ':param12345')
->willReturn('tstamp < :param12345');
$this->expressionBuilder->expects(self::once())
->method('gt')
->with('tstamp', ':param0')
->willReturn('tstamp > :param0');
$this->queryBuilder->method('where')->willReturnSelf();

$this->queryBuilder->expects(self::once())
Expand All @@ -163,4 +169,46 @@ public function testDeleteEntriesLastSeenBefore(): void

self::assertSame(7, $this->subject->deleteEntriesLastSeenBefore(12345));
}

public function testCountEntriesWithMissingTimestamp(): void
{
$this->queryBuilder->expects(self::once())
->method('count')
->with('uid')
->willReturnSelf();
$this->queryBuilder->method('from')->willReturnSelf();
$this->queryBuilder->expects(self::once())
->method('createNamedParameter')
->with(0, Connection::PARAM_INT)
->willReturn(':zero');
$this->expressionBuilder->method('eq')
->with('tstamp', ':zero')
->willReturn('tstamp = :zero');
$this->queryBuilder->method('where')->willReturnSelf();

$result = $this->createMock(Result::class);
$result->expects(self::once())
->method('fetchOne')
->willReturn('4');

$this->queryBuilder->expects(self::once())
->method('executeQuery')
->willReturn($result);

self::assertSame(4, $this->subject->countEntriesWithMissingTimestamp());
}

public function testInitializeMissingTimestampsSetsCurrentTimeAndReturnsAffectedRows(): void
{
$this->connection->expects(self::once())
->method('update')
->with(
'tx_typo3loginwarning_iplog',
self::callback(static fn (array $data): bool => is_int($data['tstamp']) && $data['tstamp'] > 0),
['tstamp' => 0],
)
->willReturn(4);

self::assertSame(4, $this->subject->initializeMissingTimestamps());
}
}