[BUGFIX] Use the return value of the UPDATE statement correctly in the LastSearchesRepository::update method

[RZivaralam]

To Reproduce

This confirms that the exception is triggered in valid and expected situations, for example when:

multiple users perform the same search query at nearly the same time, or

the same search term is executed repeatedly within the same second.

In these cases:

the UPDATE statement does find the row (correct sequence_id)

but writes identical values (keywords and second-based tstamp)

MySQL / MariaDB correctly returns affectedRows = 0

the current logic incorrectly interprets this as “row not found”

[dkd-kaehm]

Are you sure about that statement?:

---

There is no boolean return value on TYPO3 12 and 13 LTS Stacks.

[RZivaralam]

Let me explain again what I tried to change and why.

Yes, you are right: executeStatement() returns an integer, not a boolean. I assumed that executeStatement() might return something like false or -1 if an UPDATE did not work correctly,
so I suggested checking for a failure condition.
However, the real problem is not the return type itself, but how the value is interpreted.

The current code treats affectedRows < 1 as an error and throws an exception. This is wrong.

A return value of 0 does not mean that the sequence_id does not exist. It simply means that the UPDATE was executed successfully, but nothing changed. This is a valid and very common situation.

This happens especially when:

• multiple users trigger searches at the same time
• the same search term is executed concurrently
• the data (e.g., timestamp or keywords) remains the same

In these cases, affectedRows will be 0, and an exception will be thrown, even though nothing is wrong.

I understand that the intention of this check is to ensure that the sequence_id exists in the table. But this check is redundant.

I manually changed the database (renamed or altered the sequence_id column) to verify this:
If the sequence_id does not exist or the column is invalid, TYPO3 already throws a database error. This is correct and sufficient.
l renamed sequence_id to sequence_id1

Adding an extra PHP-level check for this causes:

• redundant validation
• false positives
• race conditions when multiple users update at the same time

So the issue is not that the return value is an integer.
The issue is that 0 is treated as an error, although it is a valid result.

executeStatement() returning an integer already guarantees that the SQL was executed successfully.
Column existence and type safety are enforced by the database.A return value of 0 is a valid state and must not be treated as an error.

In my opinion, the affectedRows < 1 check should be removed or relaxed; otherwise, concurrent searches will continue to trigger unnecessary exceptions.

This code should be deleted:
if ($affectedRows < 1) {
throw new InvalidArgumentException(vsprintf('By trying to update last searches row with values "%s" nothing was updated, make sure the given "sequence_id" exists in database.', [json_encode($lastSearchesRow)]), 1502717923);
}

[RZivaralam]

You can also reproduce this issue quite easily.

Try searching for the same term multiple times in quick succession
(e.g. pressing Enter repeatedly on the same search query).

With concurrent or very fast repeated searches, the UPDATE may legitimately
return 0 affected rows, and the current affectedRows < 1 check will then
throw an exception.

This demonstrates that the issue is not theoretical, but can be reproduced
in real usage scenarios with normal user behavior.

Thanks for taking the time to look into this.

[dkd-kaehm]

@RZivaralam
Is there a way to add an integration test case for that?
I still do not get why we need that.

[RZivaralam]

To reproduce the issue:

1. Set plugin.tx_solr.search.lastSearches.mode = global, so the last searches are stored in the database.
2. Search for the same keyword several times in the frontend.
3. After a few searches, the update statement does not update any row anymore and returns 0 affectedRows < 1
4. The current check interprets this as if the column sequence_id does not exist.
5. But in this case, the column exists. The real meaning is only that there was no new update to perform.

So the problem is not that sequence_id is missing. The problem is that the current if condition interprets a “no update” result as “column does not exist”.

In my opinion, this additional exception is not necessary. If the column sequence_id really does not exist, the database layer would already throw an SQL error like Unknown column 'sequence_id'.

So the check should either be removed or changed, because affectedRows < 1 here does not necessarily mean that the column is missing.