Conversation
A static variable inside an instance method is bound to the method, not to the object. As soon as the first visitor yielded a non-null value, every subsequent visitor in the same PHP process returned that same Pagevisit. The intended per-object caching never happened. This silently corrupted scoring: ScoringService uses the getter in getNumberOfDaysSinceLastVisit(), and LuxServiceRecalculateScoringCommand iterates all visitors in a single process, so lastVisitDaysAgo was wrong for every visitor but the first. It also made the lead summary mail show one single timestamp for all leads. getPagevisitLast() already performs exactly the same lookup without the static, so the method now delegates to it. This removes the bug and the duplicated logic at once.
Before `static` was used inside the getter. Unfortunately `static` is bound to the method and not to the individual object. When calling `getLastPagevisit()` on object a and calling it later again on object b the cached value of object a was returned. To fix this, set a dynamic property for the model so the value is cached individually. Additionally, tests were added to check if the value is cached and if the value leaks between visitor objects. fixes: in2code-de#81
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
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.
What
Visitor::getLastPagevisit()caches its result in astaticvariable. A static variable inside aninstance method is bound to the method, not to the object, so as soon as the first visitor yields a
non-null value, every subsequent visitor in the same PHP process receives that same
Pagevisit.getPagevisitLast()already performs exactly the same lookup without the static, so the method nowdelegates to it. This removes the bug and the duplicated logic at once.
Why it matters
Scoring is silently corrupted.
ScoringService::getNumberOfDaysSinceLastVisit()uses the getter,and
LuxServiceRecalculateScoringCommanditerates all visitors in a single process:After the first visitor that has any page visit, every remaining visitor is scored with that
visitor's
lastVisitDaysAgo, which the default formula subtracts directly. A wrong value is writtento the database for the whole visitor table.
The lead summary mail shows one timestamp for all leads, because
getDateOfLastVisit()delegatesto the same getter.
Evidence
Three different visitors, each with their own page visits, on 44.3.0:
Before the change
getLastPagevisit()returns 1 distinct value for the three visitors, after itreturns 3 — verified against 44.3.0 in a TYPO3 13.4 / PHP 8.2 installation.
A summary mail generated on the same installation showed 11 distinct "last visit" pages across 35
leads but only 1 distinct timestamp.
Notes
If the caching was intentional, a plain instance property would achieve it correctly — happy to
change the implementation if you prefer that.