[loevgaard]

Based on #13. This PR only adds its own commits on top of it. Found by the functional tests in #13.

The bug

The README says to set metadata_class to your own class "to add mapped fields or behaviour", and Entity\Metadata is deliberately not final so it can be extended. But an application that does this doesn't boot:

Entity class 'App\Entity\ClientMetadata' is a subclass of the root entity class 'Setono\ClientBundle\Entity\Metadata', but no inheritance mapping type was declared.

The bundle maps its Metadata as a mapped superclass. ConvertToEntityListener turned it, and any subclass, into an entity no matter which class was configured, so Doctrine saw the application's entity as a subclass of another entity.

The fix

SetonoClientExtension now decides at build time whether the bundle's Metadata becomes an entity:

• With the default configuration, or a custom class that only implements MetadataInterface, it keeps ConvertToEntityListener, which turns the bundle's Metadata into an entity, as before.
• With a custom class that extends Metadata, it removes the listener. The bundle's class stays a mapped superclass, and the application's entity inherits its clientId and metadata fields.

The listener now only touches the bundle's own class, never an application class.

The extension also stores the declared name of the configured class in setono_client.metadata_class. A value like \App\Entity\ClientMetadata passed the validation, but Doctrine couldn't find the class by that name.

The README's configuration reference now says how to extend Metadata, and that the subclass doesn't inherit the table name: an application that already stores metadata maps its entity to setono_client__metadata to keep it.

Tests

• CustomMetadataClassTest extends MetadataTest, so all metadata tests also run in a custom_metadata environment, against the application's own entity mapped to app_client_metadata. It also checks that the metadata goes to that table, and that the bundle's Metadata stays a mapped superclass. Without the fix, the kernel can't even warm its cache.
• The test that the mapping is valid and matches the schema moved from DoctrineMappingTest to MetadataTest, so it runs in both environments.
• tests/bootstrap.php also creates the schema of the custom_metadata environment. That environment gets its own DAMA connection key and no static metadata cache: DAMA shares both between kernels by name, but this environment has its own database and maps the bundle's Metadata differently.
• The extension tests cover removing the listener for a subclass, keeping it for a class that only implements the interface, and the declared class name. The listener tests cover the bundle's class and other classes.

Testing

• PHPUnit: 96 tests pass on PHP 8.4 with Symfony 7.4. Merged with Reset the cached client between requests and key the cookie cache by request #15 and Reject unusable client ids and look up the entity manager lazily #20, they also pass with the lowest dependencies installed like CI does, on PHP 8.1 with Symfony 6.4 and on PHP 8.2 with Symfony 7.0
• PHPStan, ECS and composer-dependency-analyser pass
• Infection: 81 of 81 mutants killed

Stack

PRs that add tests to the bundle and fix what the tests and their review found. #12 is merged. Review and merge the rest in this order. #14, #15 and #20 are independent of each other and all build on #13.

1. Move the tests into tests/Unit and add missing unit tests #12 (merged): move the existing tests into tests/Unit and add the unit tests that were missing
2. Add functional tests that run the bundle with a database #13: functional tests that run the bundle in a Symfony application with a database
3. Fix configuring a metadata class that extends the bundle's Metadata #14: fix a custom metadata_class that extends the bundle's Metadata, found by the functional tests
4. Reset the cached client between requests and key the cookie cache by request #15: reset the cached client between requests in long-running runtimes, and key the cookie cache by request, found by the functional tests
5. Reject unusable client ids and look up the entity manager lazily #20: reject unusable client ids and look up the entity manager lazily, found in the review of Move the tests into tests/Unit and add missing unit tests #12 and Add functional tests that run the bundle with a database #13

[loevgaard]

Extending Metadata silently moves the data to a new table, and the migration diff drops the old one

The child entity inherits the fields from the mapped superclass but not its table. Doctrine only copies $parent->table for single-table inheritance (ClassMetadataFactory::doLoadMetadata). So <entity name="App\Entity\ClientMetadata"/> maps to ClientMetadata/client_metadata, not setono_client__metadata. I checked this with a probe: the child's table name came out as the short class name.

Take an existing app that switches from the default class to a subclass, as this text suggests. doctrine:migrations:diff then creates the new table and drops setono_client__metadata, because that table is now only a mapped superclass. All stored client metadata is lost. Please say here, and in the "table setono_client__metadata" note near the top of the README, that the entity must set table="setono_client__metadata" to keep existing data.

[loevgaard]

Done in 2cbad1f: the README says that the subclass doesn't inherit the table name, and to map it to table="setono_client__metadata" to keep the stored metadata.

[loevgaard]

Behaviour change for apps whose custom metadata_class only implements MetadataInterface

Before, the is_a(..., Metadata::class, true) check always turned the bundle's Metadata into an entity, whatever was configured. So setono_client__metadata was always part of the schema. With exact matching, an app that set a non-extending custom class now gets that table dropped by the next doctrine:migrations:diff / schema:update. Any data still in it is lost, for example from before the app switched classes.

Code that still queries Setono\ClientBundle\Entity\Metadata::class directly (reports, commands) now hits a mapped superclass and fails. Neither the README nor an upgrade note mentions this, and README line 57 still says the bundle maps a Metadata entity with table setono_client__metadata.

[loevgaard]

Fixed in 2dcb685: the extension only removes the listener when the configured class extends the bundle's Metadata. With a class that only implements the interface, the bundle's Metadata stays an entity, as before, so its table stays in the schema. The extension test covers both cases.

[loevgaard]

Turning an app-owned mapped superclass into an entity here skips the ORM's entity-only processing

loadClassMetadata runs at the end of ClassMetadataFactory::doLoadMetadata, after the if (! $class->isMappedSuperclass) block. That block does the missingInheritanceTypeDeclaration check and inlines embeddables. The new it_turns_a_configured_metadata_class_into_an_entity test makes it supported to convert a configured app class that is mapped as <mapped-superclass>. But if that class has an <embedded>, it becomes an entity whose embedded fields were never inlined. I checked this with a probe: getFieldNames() returns only metadata, clientId, so the embedded columns are missing from the schema and never persisted.

This was safe as long as the only class flipped was the bundle's own Metadata (no parent, no embeddables). Consider only flipping Metadata::class, and expecting apps to map their own class as an entity.

[loevgaard]

Fixed in 2dcb685: the listener no longer turns application classes into entities. It only converts the bundle's own Metadata, as before this PR, and that class has no embeddables or inheritance, so the steps Doctrine skips don't matter for it.

[loevgaard]

Altitude: decide this at container-build time instead of matching on every class load

The only class that needs flipping is the bundle's own mapped superclass, and only when it's the configured class. SetonoClientExtension::load() already knows metadata_class. It could register this listener only when metadata_class === Metadata::class, or remove the definition otherwise, and the listener could keep matching Metadata::class without a constructor argument.

That way the listener never touches app-owned mappings (see the comment on the listener). It also isn't attached to every entity manager and called for every class when a custom class is configured, and the "configured class" coupling isn't needed.

[loevgaard]

Done in 2dcb685, as suggested: the extension removes the listener when the configured class extends Metadata, and the listener only targets Metadata::class, without a constructor argument.

[loevgaard]

Test gap: only the extending-class case is covered, and only through 4 tests

The PR changes behaviour in two directions, but there is no functional test where metadata_class implements MetadataInterface without extending Metadata. That case should show the bundle's Metadata dropping out of the schema and storage going through the custom entity. Also, the custom environment only runs these 4 tests. The rest of the metadata behaviour (remove, lazy no-query, and the concurrent-insert UniqueConstraintViolationException path in the persister) is never run against a custom entity/table. Consider running MetadataTest in both environments, for example with a subclass that overrides getEnvironment().

[loevgaard]

Done in 74f83e9: CustomMetadataClassTest extends MetadataTest, so the whole metadata suite also runs against the custom entity and its table, including removal, expiry, the test without queries and the concurrent insert. For a class that only implements the interface, nothing changes compared to before this PR. The extension test checks that the listener stays registered then.

[loevgaard]

getEnvironment() now always forces test and ignores APP_ENV

Before, static::createClient() without options let KernelTestCase::createKernel() resolve the environment as $_ENV['APP_ENV'] ?? $_SERVER['APP_ENV'] ?? 'test'. Hard-coding 'test' here means APP_ENV from the environment or phpunit.xml no longer has any effect on the base suites. A fallback such as $_ENV['APP_ENV'] ?? $_SERVER['APP_ENV'] ?? 'test', with a string check for PHPStan, keeps the old behaviour.

[loevgaard]

That's intended. #13 now always boots the test environment with debug on (1149e2f), because the Symfony CLI's php sets APP_ENV=dev. getEnvironment() only lets CustomMetadataClassTest pick its own environment.

[loevgaard]

Duplicated test, and a near-tautological assertion

This method is a verbatim copy of DoctrineMappingTest::its_mapping_is_valid_and_matches_the_database_schema. It could live once in FunctionalTestCase (or a trait) so every environment gets it. Also, schemaInSyncWithMetadata() can hardly fail here, because setUp() just created the schema from getAllMetadata(). Only validateMapping() actually guards something.

[loevgaard]

Done in 74f83e9: the test lives in MetadataTest only, so it runs in both environments. I kept schemaInSyncWithMetadata(). It fails when the mapping makes Doctrine see a difference to the schema it just created, e.g. a column definition that the platform normalizes differently. Then every doctrine:migrations:diff would generate a migration. The docblock now says so.

[loevgaard]

Fixture borrowed across test areas

The listener test imports the DI test fixture Tests\Unit\DependencyInjection\Fixture\CustomMetadata, and the PR adds a second, unrelated CustomMetadata under tests/Functional/App/Entity. Moving the unit fixture to a shared tests/Unit/Fixture namespace, or giving this test its own fixture, keeps the DI tests' fixture from silently becoming a dependency of the listener tests.

[loevgaard]

Done in 2dcb685: the listener tests no longer need a custom class, so they don't use the fixture.

[loevgaard]

Exact string match is stricter than the extension's validation

SetonoClientExtension::load() validates metadata_class with is_a(..., true). That check accepts a leading backslash and any letter case (PHP class lookup ignores both), but getName() here is the canonical name. A config such as metadata_class: '\Setono\ClientBundle\Entity\Metadata' passes validation, yet the bundle's class is never turned into an entity, and the failure only shows later at query/schema time. The old is_a() check tolerated this. Normalising (ltrim($class, '\\')) or requiring the canonical name in the extension would turn it into a boot-time error.

[loevgaard]

Fixed in 2dcb685: the listener no longer compares names with the configured class. The extension also stores the declared name of the class (ReflectionClass::getName()) in setono_client.metadata_class, so a leading backslash doesn't break the persister and the provider either. Both look the class up in Doctrine by that name.