Skip to content

Commit 142fd65

Browse files
johanibjohanib
committed
Use value object as return in IdPSplitter
Prior to this change, bugs could occur because the array returned rom the IdP Splitter exposed all data. This change adds a value object in the return, so logic can be better encapsulated.
1 parent 93c0e0b commit 142fd65

4 files changed

Lines changed: 76 additions & 33 deletions

File tree

‎src/OpenConext/EngineBlock/Service/Wayf/IdpSplitter.php‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,11 @@ final class IdpSplitter
2626
*
2727
* @param WayfIdp[] $idpList Full transformed IdP list
2828
* @param array $preferredEntityIds Ordered list of entity IDs to feature at the top
29-
* @return array{preferred: WayfIdp[], regular: WayfIdp[]}
3029
*/
31-
public function split(array $idpList, array $preferredEntityIds): array
30+
public function split(array $idpList, array $preferredEntityIds): WayfSplitResult
3231
{
3332
if (empty($preferredEntityIds)) {
34-
return ['preferred' => [], 'regular' => $idpList];
33+
return new WayfSplitResult(preferred: [], regular: $idpList);
3534
}
3635

3736
$orderMap = array_flip($preferredEntityIds);
@@ -53,6 +52,6 @@ public function split(array $idpList, array $preferredEntityIds): array
5352
$mergeArgs = array_values(array_filter($preferredBuckets));
5453
$preferred = empty($mergeArgs) ? [] : array_merge(...$mergeArgs);
5554

56-
return ['preferred' => $preferred, 'regular' => $regular];
55+
return new WayfSplitResult(preferred: $preferred, regular: $regular);
5756
}
5857
}

‎src/OpenConext/EngineBlock/Service/Wayf/WayfSplitResult.php‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
<?php
2+
3+
/**
4+
* Copyright 2026 SURFnet B.V.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
declare(strict_types=1);
20+
21+
namespace OpenConext\EngineBlock\Service\Wayf;
22+
23+
final class WayfSplitResult
24+
{
25+
/**
26+
* @param WayfIdp[] $preferred
27+
* @param WayfIdp[] $regular
28+
*/
29+
public function __construct(
30+
public readonly array $preferred,
31+
public readonly array $regular,
32+
) {
33+
}
34+
35+
public function hasPreferred(): bool
36+
{
37+
return !empty($this->preferred);
38+
}
39+
40+
public function containsInPreferred(string $entityId): bool
41+
{
42+
return array_any($this->preferred, static fn($idp) => $idp->entityId === $entityId);
43+
}
44+
}

‎src/OpenConext/EngineBlockBundle/Service/WayfRenderer.php‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,18 +52,17 @@ public function render(
5252
ServiceProvider $serviceProvider,
5353
): string {
5454
$split = $this->splitter->split($idpList, $preferredIdpEntityIds);
55-
$showPreferredIdps = !empty($split['preferred']);
56-
$preferredEntityIdsShown = array_map(static fn(WayfIdp $idp) => $idp->entityId, $split['preferred']);
57-
$isDefaultIdpPreferred = in_array($defaultIdpEntityId, $preferredEntityIdsShown, true);
55+
$showPreferredIdps = $split->hasPreferred();
56+
$isDefaultIdpPreferred = $split->containsInPreferred($defaultIdpEntityId);
5857

5958
$showIdPBanner = $shouldDisplayBanner
6059
&& $this->isDefaultIdpPresent($idpList)
6160
&& (!$showPreferredIdps || !$isDefaultIdpPreferred);
6261

6362
$viewModel = $this->factory->create(
6463
idpList: $idpList,
65-
regularIdpList: $split['regular'],
66-
preferredIdpList: $split['preferred'],
64+
regularIdpList: $split->regular,
65+
preferredIdpList: $split->preferred,
6766
showPreferredIdps: $showPreferredIdps,
6867
action: $action,
6968
greenHeader: $serviceProvider->getDisplayName($currentLocale),

‎tests/unit/OpenConext/EngineBlock/Service/Wayf/IdpSplitterTest.php‎

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
use OpenConext\EngineBlock\Service\Wayf\IdpSplitter;
2222
use OpenConext\EngineBlock\Service\Wayf\WayfIdp;
23+
use OpenConext\EngineBlock\Service\Wayf\WayfSplitResult;
2324
use PHPUnit\Framework\Attributes\DataProvider;
2425
use PHPUnit\Framework\TestCase;
2526

@@ -32,7 +33,7 @@ protected function setUp(): void
3233
$this->splitter = new IdpSplitter();
3334
}
3435

35-
private function split(array $idpList, array $preferredEntityIds): array
36+
private function split(array $idpList, array $preferredEntityIds): WayfSplitResult
3637
{
3738
return $this->splitter->split($idpList, $preferredEntityIds);
3839
}
@@ -57,8 +58,8 @@ public function testEmptyPreferredEntityIdsReturnsFullListAsRegular(): void
5758

5859
$result = $this->split($idpList, []);
5960

60-
$this->assertSame([], $result['preferred']);
61-
$this->assertSame($idpList, $result['regular']);
61+
$this->assertSame([], $result->preferred);
62+
$this->assertSame($idpList, $result->regular);
6263
}
6364

6465
public function testPreferredIdpIsMovedOutOfRegularList(): void
@@ -68,10 +69,10 @@ public function testPreferredIdpIsMovedOutOfRegularList(): void
6869

6970
$result = $this->split([$idp1, $idp2], ['https://idp1.example.org']);
7071

71-
$this->assertCount(1, $result['preferred']);
72-
$this->assertSame('https://idp1.example.org', $result['preferred'][0]->entityId);
73-
$this->assertCount(1, $result['regular']);
74-
$this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId);
72+
$this->assertCount(1, $result->preferred);
73+
$this->assertSame('https://idp1.example.org', $result->preferred[0]->entityId);
74+
$this->assertCount(1, $result->regular);
75+
$this->assertSame('https://idp2.example.org', $result->regular[0]->entityId);
7576
}
7677

7778
public function testConfiguredOrderIsPreservedInPreferredList(): void
@@ -85,11 +86,11 @@ public function testConfiguredOrderIsPreservedInPreferredList(): void
8586
['https://idp3.example.org', 'https://idp1.example.org']
8687
);
8788

88-
$this->assertCount(2, $result['preferred']);
89-
$this->assertSame('https://idp3.example.org', $result['preferred'][0]->entityId);
90-
$this->assertSame('https://idp1.example.org', $result['preferred'][1]->entityId);
91-
$this->assertCount(1, $result['regular']);
92-
$this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId);
89+
$this->assertCount(2, $result->preferred);
90+
$this->assertSame('https://idp3.example.org', $result->preferred[0]->entityId);
91+
$this->assertSame('https://idp1.example.org', $result->preferred[1]->entityId);
92+
$this->assertCount(1, $result->regular);
93+
$this->assertSame('https://idp2.example.org', $result->regular[0]->entityId);
9394
}
9495

9596
public function testDisconnectedPreferredIdpIsExcludedFromBothLists(): void
@@ -102,10 +103,10 @@ public function testDisconnectedPreferredIdpIsExcludedFromBothLists(): void
102103
['https://idp2.example.org']
103104
);
104105

105-
$this->assertSame([], $result['preferred']);
106+
$this->assertSame([], $result->preferred);
106107
// idp1 is not preferred so it stays in regular; idp2 is preferred but disconnected > excluded from both
107-
$this->assertCount(1, $result['regular']);
108-
$this->assertSame('https://idp1.example.org', $result['regular'][0]->entityId);
108+
$this->assertCount(1, $result->regular);
109+
$this->assertSame('https://idp1.example.org', $result->regular[0]->entityId);
109110
}
110111

111112
public function testMultipleDiscoveryEntriesForSameEntityIdAreGroupedInOrder(): void
@@ -119,11 +120,11 @@ public function testMultipleDiscoveryEntriesForSameEntityIdAreGroupedInOrder():
119120
['https://idp1.example.org']
120121
);
121122

122-
$this->assertCount(2, $result['preferred']);
123-
$this->assertSame('https://idp1.example.org', $result['preferred'][0]->entityId);
124-
$this->assertSame('https://idp1.example.org', $result['preferred'][1]->entityId);
125-
$this->assertCount(1, $result['regular']);
126-
$this->assertSame('https://idp2.example.org', $result['regular'][0]->entityId);
123+
$this->assertCount(2, $result->preferred);
124+
$this->assertSame('https://idp1.example.org', $result->preferred[0]->entityId);
125+
$this->assertSame('https://idp1.example.org', $result->preferred[1]->entityId);
126+
$this->assertCount(1, $result->regular);
127+
$this->assertSame('https://idp2.example.org', $result->regular[0]->entityId);
127128
}
128129

129130
public function testPreferredEntityIdNotInIdpListIsIgnored(): void
@@ -132,8 +133,8 @@ public function testPreferredEntityIdNotInIdpListIsIgnored(): void
132133

133134
$result = $this->split([$idp1], ['https://nonexistent.example.org']);
134135

135-
$this->assertSame([], $result['preferred']);
136-
$this->assertCount(1, $result['regular']);
136+
$this->assertSame([], $result->preferred);
137+
$this->assertCount(1, $result->regular);
137138
}
138139

139140
public static function fiveScenarioProvider(): array
@@ -162,8 +163,8 @@ public function testFiveScenarioBannerAndPreferredVisibility(
162163

163164
$split = $this->split($idpList, $preferredEntityIds);
164165

165-
$showPreferred = !empty($split['preferred']);
166-
$isDefaultPreferred = in_array($defaultIdpEntityId, $preferredEntityIds, true);
166+
$showPreferred = $split->hasPreferred();
167+
$isDefaultPreferred = $split->containsInPreferred($defaultIdpEntityId);
167168
$showBanner = $defaultIdpConnected && (!$showPreferred || !$isDefaultPreferred);
168169

169170
$this->assertSame($expectShowPreferred, $showPreferred, 'showPreferredIdps mismatch');

0 commit comments

Comments
 (0)