Skip to content

Commit 9098652

Browse files
committed
Update CLAUDE.md: mark fixes done, update #118/#170 status
1 parent b438d60 commit 9098652

1 file changed

Lines changed: 9 additions & 47 deletions

File tree

CLAUDE.md

Lines changed: 9 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -562,67 +562,27 @@ $topicBuilder->onRoutesCreated(function (array $childBuilders) use ($intro) {
562562

563563
These are known open issues with enough context to resume work without re-investigation.
564564

565-
### `CwaFixtureBuilder.component()` throws for non-timestamped entities — fix needed
565+
### ~~`CwaFixtureBuilder.component()` throws for non-timestamped entities~~FIXED
566566

567-
**Context (discovered 2026-06-19):** `CwaFixtureBuilder.phaseOne()` unconditionally calls `TimestampedDataPersister->persistTimestampedFields($component, true)` for every `ComponentBuilder` registered via `->component()`. This throws `InvalidArgumentException` (`AttributeReader.findAttributeConfiguration` line 104) when the entity does not have the `#[Timestamped]` annotation. `AbstractComponent` subclasses (e.g. `HtmlContent`, `NavigationLink`, `Image`) use `#[Publishable]` but NOT `#[Timestamped]`, so `$cwa->component($htmlContent)` always crashes.
568-
569-
**Relevant code:** `src/Fixture/CwaFixtureBuilder.php` phaseOne loop (~line 316–323):
570-
```php
571-
foreach ($this->componentBuilders as $componentBuilder) {
572-
$component = $componentBuilder->getComponent();
573-
$this->timestampedPersister->persistTimestampedFields($component, true); // throws for HtmlContent etc.
574-
$this->persistWithAssociations($component);
575-
...
576-
}
577-
```
578-
579-
**Fix required:**
580-
581-
1. Add `isConfigured(object|string $entity): bool` to `TimestampedDataPersister` (delegates to `$this->annotationReader->isConfigured($entity)``TimestampedAttributeReader` inherits `isConfigured` from `AttributeReader`, which catches `InvalidArgumentException` and returns false).
582-
583-
2. In `phaseOne()`, guard the call:
584-
```php
585-
if ($this->timestampedPersister->isConfigured($component)) {
586-
$this->timestampedPersister->persistTimestampedFields($component, true);
587-
}
588-
```
589-
590-
The `Layout`, `Page`, and `AbstractPageData` loops in phaseOne are unaffected — those entities all have `#[Timestamped]`. Only the `componentBuilders` loop needs the guard.
591-
592-
**Workaround for `AppScaffold` until fixed:** Use `$cwa->persist($htmlContent)` (which calls `persistWithAssociations` without any timestamp logic) for standalone components set as PageData properties. This is the same effect as the intended `$cwa->component()` call minus the timestamp step (which is a no-op for these entities anyway since they have no `createdAt`/`modifiedAt` fields).
567+
**Fixed (commit `b438d60d`):** `TimestampedDataPersister.isConfigured()` added; `CwaFixtureBuilder.phaseOne()` now guards the `persistTimestampedFields` call with `isConfigured()`. Non-timestamped components (e.g. `HtmlContent`, `NavigationLink`) are persisted without crashing. Timestamped components (if any) still get timestamps set. Unit test added.
593568

594569
---
595570

596-
### ComponentPosition `sortValue` collision on insert — API-side normalisation needed
597-
598-
**Context (from Nuxt module):** When an admin inserts a component "before" or "after" an existing position, the module computes a `sortValue` for the new `ComponentPosition` that may equal an existing position's `sortValue`. For "add before X (sortValue=N)", the new position gets `sortValue=N` (same as X). For "add after X (sortValue=N)", the new position gets `sortValue=N+1` which may collide with the position immediately following X.
599-
600-
**Module-side workaround (shipped):** Before POSTing the new component, the module PATCHes all positions in the group with `sortValue >= newSortValue`, incrementing each by 1 (in descending order to avoid intermediate collisions). This is sequential HTTP requests and adds latency.
571+
### ~~ComponentPosition `sortValue` collision on insert — double-shifting~~ — FIXED
601572

602-
**Preferred API-side fix:** The API should accept an atomic "insert before/after" parameter on `ComponentPosition` POST that handles sort value shifting in a single database transaction. Options:
603-
604-
1. **`insertBefore: IRI`** — the API shifts all positions with `sortValue >= targetPosition.sortValue` by +1, then assigns the freed `sortValue` to the new position.
605-
2. **`insertAfter: IRI`** — the API shifts all positions with `sortValue > targetPosition.sortValue` by +1, assigns `targetPosition.sortValue + 1` to the new position.
606-
3. **Auto-shift on collision** — when a `ComponentPosition` is written with a `sortValue` that already exists in the group, automatically shift all conflicting positions up before saving. No new parameter needed; the API resolves collisions transparently.
607-
608-
Option 3 is simplest: no API contract change, backward-compatible. The module could then remove its pre-PATCH shifting step and rely on the API to handle collisions atomically.
609-
610-
**Where to implement:** `ComponentPositionEventListener`, `ComponentPositionStateProcessor`, or a Doctrine `prePersist` event on `ComponentPosition`. The shift must be transactional (all updates in the same flush as the insert).
573+
**Fixed (commit `b438d60d`):** `ComponentPositionSortValueHelper.calculateSortValue()` now only shifts existing positions when an actual sortValue collision exists. Previously it always shifted all positions with `sortValue >= newSortValue`, causing double-shifts when the Nuxt module pre-shifted upstream. Behat test added for the no-collision case. The module's pre-shift workaround remains compatible (no collision → no shift).
611574

612575
Related: Nuxt module issue `components-web-app/cwa-nuxt-module#224` Bug 2.
613576

614577
---
615578

616579
### #170 — Component group `allowedComponents` does not validate `pageDataProperty` positions on write
617580

618-
**Read side fixed** (commit `2305ad89`): `ComponentPositionNormalizer.normalizeForPageData()` now skips populating the component if the resolved type is not in `componentGroup.allowedComponents`. The position remains in `componentPositions` but with `component = null`. Direct-component write-side validation already works via `ComponentPositionValidator`.
581+
**Read side fixed** (commit `2305ad89`): `ComponentPositionNormalizer.normalizeForPageData()` now skips populating the component if the resolved type is not in `componentGroup.allowedComponents`. Direct-component write-side validation already works via `ComponentPositionValidator`.
619582

620583
**`pageDataProperty` write-side still open:** When creating a `ComponentPosition` with `pageDataProperty` set, no validation is done against `allowedComponents`. The property name is just a string — the component type isn't known until render time.
621584

622-
**Options:**
623-
- Validate that the PageData class's property type for `pageDataProperty` is in `allowedComponents` (requires knowing the PageData class at write time)
624-
- Accept the current read-side filtering as sufficient (disallowed types are hidden even if they sneak in)
625-
- Add a separate `allowedPageDataProperties` restriction
585+
**Agreed plan:** The module must send `pageDataClass` (FQCN of the PageData entity, e.g. `"App\\Entity\\ConferenceData"`) alongside `pageDataProperty` in POST/PATCH. The API can then resolve the property type and validate against `allowedComponents`. Requires coordinated change in both projects. See issue #170 (reopened) and nuxt module CLAUDE.md section on `allowedComponents` for the full spec.
626586

627587
Related Nuxt module issue: `components-web-app/cwa-nuxt-module#151`.
628588

@@ -656,7 +616,9 @@ A console command to scaffold a new component class would ease the process and a
656616

657617
An admin viewing a component group or draft resource currently has no way to know how many draft items exist across locations. A count per-location would allow the admin UI to surface "3 unpublished items in this group" without fetching all items.
658618

659-
**No implementation started.** Likely a custom API endpoint or serialization group addition that returns aggregated counts. Needs design work on the response shape.
619+
**Partially implemented (commit `b438d60d`):** `_metadata.publishable.locationCount` is now returned in component responses, showing how many `ComponentPosition` entries reference that component. Behat test in `features/publishable/publishable.feature`.
620+
621+
**Remaining:** aggregated counts per location (e.g. "N draft items in this group") and collection-level counts are not yet implemented.
660622

661623
---
662624

0 commit comments

Comments
 (0)