Repository navigation
Reject unusable client ids and look up the entity manager lazily - #20
Merged
Merged
Conversation
This was referenced Oct 26, 2026
loevgaard
force-pushed
the
tests/functional-suite
branch
from
October 26, 2026 10:57
1149e2f to
bbb195f
Compare
loevgaard
force-pushed
the
fix/cookie-and-doctrine-edge-cases
branch
2 times, most recently
from
October 26, 2026 11:09
9148df3 to
15098ea
Compare
loevgaard
force-pushed
the
fix/cookie-and-doctrine-edge-cases
branch
from
October 26, 2026 11:40
15098ea to
fd5fc69
Compare
loevgaard
force-pushed
the
fix/cookie-and-doctrine-edge-cases
branch
from
October 26, 2026 11:46
fd5fc69 to
c702a12
Compare
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.
Closes #17.
The review of #12 and #13 found these bugs in code that those PRs don't change, so they get their own PR.
Empty and overlong client ids
RequestBasedCookieProvideraccepted any client id thatCookie::fromString()returned:2.1.1.gave an empty client id, so every visitor with such a cookie shared one client and one metadata row.clientIdcolumn was accepted. As A client id longer than 255 characters makes requests that write metadata fail on MySQL and PostgreSQL #17 describes, every request that wrote metadata for that visitor then failed with a 500 on MySQL and PostgreSQL.The provider now treats both as no cookie, so the visitor gets a new client id and a new cookie.
Looking up the entity manager lazily
DoctrineOrmBasedMetadataProviderlooked up the entity manager of the metadata class when it created the lazy metadata.getManagerForClass()initializes the entity manager and asks its metadata factory about the class, so every request with a cookie did that, even when the application never used the metadata. The lookup now happens in the lazy ghost's initializer.Not in this PR: the closed entity manager after a lost insert race
The review also found that the persister leaves the entity manager closed after it swallows a
UniqueConstraintViolationException(#5). I first reset the manager through the registry, but reverted that: with Symfony 7.0 and ORM 3, the reset turns the entity manager back into an uninitialized lazy ghost, and initializing it again fails on ORM's readonly properties (Cannot modify readonly property Doctrine\ORM\EntityManager::$conn). The manager is then unusable, which is worse than closed. Writing the row with DBAL instead offlush(), as #16 suggests, keeps the manager open without a reset.Tests
it_returns_lazy_metadataasserts that the provider doesn't look up the manager before the metadata is used.Testing
Stack
PRs that add tests to the bundle and fix what the tests and their review found. #12 to #15 are merged, and #20 builds on master.
tests/Unitand add the unit tests that were missingmetadata_classthat extends the bundle'sMetadata, found by the functional tests