Skip to content

Commit b438d60

Browse files
committed
Fix CwaFixtureBuilder crash for non-timestamped components, sort value collision, and component location count
- CwaFixtureBuilder.phaseOne: guard persistTimestampedFields with isConfigured() check so non-timestamped components (e.g. HtmlContent, NavigationLink) don't throw; add isConfigured() to TimestampedDataPersister as a passthrough to the attribute reader - ComponentPositionSortValueHelper: only shift existing positions when an actual sortValue collision exists; avoids double-shifting when the caller has pre-shifted upstream - ResourcePublishableMetadata/ResourceMetadata: add locationCount field populated from getComponentPositions() count so admins can see how many positions use a component - Reopen #170: write-side allowedComponents validation for pageDataProperty still needs coordinated pageDataClass field from module side
1 parent 8fda5bc commit b438d60

10 files changed

Lines changed: 108 additions & 8 deletions

File tree

features/bootstrap/DoctrineContext.php

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1558,4 +1558,32 @@ public function aRefreshTokenShouldHaveBeenGenerated(int $count): void
15581558
}
15591559
Assert::assertLessThanOrEqual(1, $nonExpiredCount, \sprintf('There should only be 1 token that is not expired. There are %d', $nonExpiredCount));
15601560
}
1561+
1562+
/**
1563+
* @Given there is a published DummyPublishableComponent in :count component positions
1564+
*/
1565+
public function thereIsAPublishedDummyPublishableComponentInPositions(int $count): void
1566+
{
1567+
$component = new DummyPublishableComponent();
1568+
$component->setPublishedAt(new \DateTime());
1569+
$this->manager->persist($component);
1570+
$this->restContext->resources['publishable_component'] = $this->iriConverter->getIriFromResource($component);
1571+
1572+
for ($i = 0; $i < $count; ++$i) {
1573+
$componentGroup = new ComponentGroup();
1574+
$componentGroup->reference = 'test_group_' . $i;
1575+
$componentGroup->location = 'test_group_' . $i;
1576+
$this->timestampedHelper->persistTimestampedFields($componentGroup, true);
1577+
$this->manager->persist($componentGroup);
1578+
1579+
$position = new ComponentPosition();
1580+
$position->sortValue = 0;
1581+
$position->component = $component;
1582+
$position->componentGroup = $componentGroup;
1583+
$this->timestampedHelper->persistTimestampedFields($position, true);
1584+
$this->manager->persist($position);
1585+
}
1586+
1587+
$this->manager->flush();
1588+
}
15611589
}

features/main/component_position.feature

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,3 +134,27 @@ Feature: Component positions
134134
And I send a "GET" request to the resource "position_3"
135135
And the response status code should be 200
136136
And the JSON node "sortValue" should be equal to the number 3
137+
138+
@loginUser
139+
Scenario: Inserting a ComponentPosition at a sortValue with no collision does not shift existing positions
140+
Given there is a ComponentGroup with 3 components
141+
When I send a "POST" request to "/_/component_positions" with data:
142+
| componentGroup | component | sortValue |
143+
| resource[component_group] | resource[component_0] | 5 |
144+
Then the response status code should be 201
145+
And the JSON node "sortValue" should be equal to the number 5
146+
And I add "Content-Type" header equal to "application/ld+json"
147+
And I add "Accept" header equal to "application/ld+json"
148+
And I send a "GET" request to the resource "position_0"
149+
And the response status code should be 200
150+
And the JSON node "sortValue" should be equal to the number 0
151+
And I add "Content-Type" header equal to "application/ld+json"
152+
And I add "Accept" header equal to "application/ld+json"
153+
And I send a "GET" request to the resource "position_1"
154+
And the response status code should be 200
155+
And the JSON node "sortValue" should be equal to the number 1
156+
And I add "Content-Type" header equal to "application/ld+json"
157+
And I add "Accept" header equal to "application/ld+json"
158+
And I send a "GET" request to the resource "position_2"
159+
And the response status code should be 200
160+
And the JSON node "sortValue" should be equal to the number 2

features/publishable/publishable.feature

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,3 +437,11 @@ Feature: Access to unpublished/draft resources should be configurable
437437
Then the response status code should be 204
438438
And the resource component_position should not exist
439439
And the resource publishable_draft should not exist
440+
441+
442+
@loginAdmin
443+
Scenario: A publishable component's location count is returned in metadata
444+
Given there is a published DummyPublishableComponent in 2 component positions
445+
When I send a "GET" request to the resource "publishable_component"
446+
Then the response status code should be 200
447+
And the JSON node "_metadata.publishable.locationCount" should be equal to the number 2

src/Fixture/CwaFixtureBuilder.php

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,9 @@ private function phaseOne(): void
315315

316316
foreach ($this->componentBuilders as $componentBuilder) {
317317
$component = $componentBuilder->getComponent();
318-
$this->timestampedPersister->persistTimestampedFields($component, true);
318+
if ($this->timestampedPersister->isConfigured($component)) {
319+
$this->timestampedPersister->persistTimestampedFields($component, true);
320+
}
319321
$this->persistWithAssociations($component);
320322
foreach ($componentBuilder->getGroupBuilders() as $groupBuilder) {
321323
$this->createAndLinkComponentGroup($groupBuilder, $component);

src/Helper/ComponentPosition/ComponentPositionSortValueHelper.php

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,21 @@ public function calculateSortValue(ComponentPosition $componentPosition, ?int $o
8787
}
8888
}
8989

90+
// Only shift existing positions if there is an actual collision at the target sortValue.
91+
// This avoids double-shifting when the caller has already pre-shifted positions upstream.
92+
$hasCollision = false;
9093
foreach ($sortCollection as $existingComponentPosition) {
91-
// for every position after this one we push it up 1
92-
if ($existingComponentPosition->sortValue >= $componentPosition->sortValue) {
93-
++$existingComponentPosition->sortValue;
94+
if ($existingComponentPosition->sortValue === $componentPosition->sortValue) {
95+
$hasCollision = true;
96+
break;
97+
}
98+
}
99+
100+
if ($hasCollision) {
101+
foreach ($sortCollection as $existingComponentPosition) {
102+
if ($existingComponentPosition->sortValue >= $componentPosition->sortValue) {
103+
++$existingComponentPosition->sortValue;
104+
}
94105
}
95106
}
96107
}

src/Helper/Timestamped/TimestampedDataPersister.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,11 @@ public function __construct(ManagerRegistry $registry, TimestampedAttributeReade
3030
$this->annotationReader = $annotationReader;
3131
}
3232

33+
public function isConfigured(object|string $entity): bool
34+
{
35+
return $this->annotationReader->isConfigured($entity);
36+
}
37+
3338
public function persistTimestampedFields(object $timestamped, bool $isNew): void
3439
{
3540
$configuration = $this->annotationReader->getConfiguration($timestamped);

src/Serializer/Normalizer/PublishableNormalizer.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,10 @@ public function normalize($object, $format = null, array $context = []): float|a
7676
}
7777

7878
$isPublished = $this->publishableStatusChecker->isActivePublishedAt($object);
79+
$locationCount = method_exists($object, 'getComponentPositions') ? count($object->getComponentPositions()) : null;
7980

8081
$resourceMetadata = $this->resourceMetadataProvider->findResourceMetadata($object);
81-
$resourceMetadata->setPublishable($isPublished);
82+
$resourceMetadata->setPublishable($isPublished, null, $locationCount);
8283

8384
$type = $object::class;
8485

src/Serializer/ResourceMetadata/ResourceMetadata.php

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,15 +121,18 @@ public function getPublishable(): ?ResourcePublishableMetadata
121121
return $this->publishable;
122122
}
123123

124-
public function setPublishable(bool $published, ?string $publishedAt = null): void
124+
public function setPublishable(bool $published, ?string $publishedAt = null, ?int $locationCount = null): void
125125
{
126126
if ($this->publishable) {
127127
$this->publishable->published = $published;
128128
$this->publishable->publishedAt = $publishedAt;
129+
if (null !== $locationCount) {
130+
$this->publishable->locationCount = $locationCount;
131+
}
129132

130133
return;
131134
}
132-
$this->publishable = new ResourcePublishableMetadata($published, $publishedAt);
135+
$this->publishable = new ResourcePublishableMetadata($published, $publishedAt, $locationCount);
133136
}
134137

135138
public function getViolations(): ?ConstraintViolationListInterface

src/Serializer/ResourceMetadata/ResourcePublishableMetadata.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ public function __construct(
2020
public bool $published,
2121
#[Groups('cwa_resource:metadata')]
2222
public ?string $publishedAt = null,
23+
#[Groups('cwa_resource:metadata')]
24+
public ?int $locationCount = null,
2325
) {
2426
}
2527
}

tests/Fixture/CwaFixtureBuilderTest.php

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,10 @@ private function makeBuilder(?ObjectManager $em = null, ?RouteGeneratorInterface
4242
/**
4343
* @param array<int, array{entity: object, isNew: bool}> $calls
4444
*/
45-
private function recordingTimestampedPersister(array &$calls): TimestampedDataPersister
45+
private function recordingTimestampedPersister(array &$calls, bool $isConfigured = true): TimestampedDataPersister
4646
{
4747
$persister = $this->createStub(TimestampedDataPersister::class);
48+
$persister->method('isConfigured')->willReturn($isConfigured);
4849
$persister->method('persistTimestampedFields')->willReturnCallback(
4950
static function (object $entity, bool $isNew) use (&$calls): void {
5051
$calls[] = ['entity' => $entity, 'isNew' => $isNew];
@@ -1562,6 +1563,21 @@ public function test_timestamped_persister_called_with_is_new_true_for_component
15621563
}
15631564
}
15641565

1566+
public function test_timestamped_persister_not_called_for_non_timestamped_component(): void
1567+
{
1568+
$calls = [];
1569+
$component = new class extends AbstractComponent {};
1570+
1571+
$builder = $this->makeBuilder(
1572+
timestampedPersister: $this->recordingTimestampedPersister($calls, isConfigured: false),
1573+
);
1574+
$builder->component($component)->group('items');
1575+
$builder->flush();
1576+
1577+
$componentCalls = array_values(array_filter($calls, static fn ($c) => $c['entity'] === $component));
1578+
$this->assertEmpty($componentCalls, 'persistTimestampedFields must NOT be called for non-timestamped components');
1579+
}
1580+
15651581
// --- LogicalAnd: routeName only stored when route IS set on entity (kills mutants 9+10) ---
15661582

15671583
public function test_named_route_for_page_not_stored_when_route_generator_does_not_set_route_on_entity(): void

0 commit comments

Comments
 (0)