Skip to content

Commit 84b6365

Browse files
bbralabbrala
committed
fix(ReplaceDialogClassOptionRector): validate before mutating the options array
Previously the rector unset `dialogClass` and reindexed the items array before checking whether the merge path could complete (e.g. existing `classes` resolves to a literal Array_, or both sides of a ui-dialog concatenation are String_ literals). When a later check failed, the rector returned null with `dialogClass` already deleted — a silent partial rewrite that lost the option entirely. Refactor splits refactor() into three phases: locate items (capturing node references, not indexes), validate the chosen merge branch, then mutate. Also bails on pathological duplicate `dialogClass` keys instead of silently rewriting one and leaving the other. Adds three no-change fixtures: non-literal new value, non-Array_ `classes`, and duplicate `dialogClass` entries.
1 parent d221548 commit 84b6365

4 files changed

Lines changed: 121 additions & 49 deletions

File tree

‎src/Drupal11/Rector/Deprecation/ReplaceDialogClassOptionRector.php‎

Lines changed: 54 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,13 @@ public function refactor(Node $node): ?Node
9292
return null;
9393
}
9494

95+
// Phase 1: locate the items and hold references (not indexes — node
96+
// references survive the later array_values() reindex).
9597
$dialogClassIdx = null;
96-
$classesIdx = null;
97-
$uiDialogIdx = null;
98+
$dialogClassValue = null;
99+
$dialogClassMatches = 0;
100+
$classesItem = null;
101+
$uiDialogItem = null;
98102

99103
foreach ($optionsArray->items as $idx => $item) {
100104
if (!$item->key instanceof String_) {
@@ -103,14 +107,16 @@ public function refactor(Node $node): ?Node
103107

104108
if ($item->key->value === 'dialogClass') {
105109
$dialogClassIdx = $idx;
110+
$dialogClassValue = $item->value;
111+
++$dialogClassMatches;
106112
} elseif ($item->key->value === 'classes') {
107-
$classesIdx = $idx;
113+
$classesItem = $item;
108114
if ($item->value instanceof Array_) {
109-
foreach ($item->value->items as $subIdx => $subItem) {
115+
foreach ($item->value->items as $subItem) {
110116
if ($subItem->key instanceof String_
111117
&& $subItem->key->value === 'ui-dialog'
112118
) {
113-
$uiDialogIdx = $subIdx;
119+
$uiDialogItem = $subItem;
114120
}
115121
}
116122
}
@@ -121,49 +127,59 @@ public function refactor(Node $node): ?Node
121127
return null;
122128
}
123129

124-
$dialogClassValue = $optionsArray->items[$dialogClassIdx]->value;
130+
// Bail on pathological duplicate `dialogClass` keys — picking one
131+
// would leave the other in place, leaving the deprecated key behind.
132+
if ($dialogClassMatches > 1) {
133+
return null;
134+
}
135+
136+
// Phase 2: validate that the chosen merge branch is safe to execute.
137+
// Done before any mutation so a failed check never leaves a
138+
// half-rewritten array (deprecated key removed, replacement skipped).
139+
// Capture narrowed values into typed locals — phase 3 reads from
140+
// these so type narrowing survives the intervening mutation.
141+
$classesInnerArray = null;
142+
$concatenatedString = null;
143+
144+
if ($classesItem !== null) {
145+
// Merging into existing `classes` requires it to resolve to a
146+
// literal array node — we cannot safely insert into a variable
147+
// or function-call result.
148+
if (!$classesItem->value instanceof Array_) {
149+
return null;
150+
}
151+
$classesInnerArray = $classesItem->value;
152+
153+
if ($uiDialogItem !== null) {
154+
// Concatenation path: both the existing ui-dialog value and
155+
// the new dialogClass value must be string literals.
156+
if (!$uiDialogItem->value instanceof String_
157+
|| !$dialogClassValue instanceof String_
158+
) {
159+
return null;
160+
}
161+
$concatenatedString = $uiDialogItem->value->value.' '.$dialogClassValue->value;
162+
}
163+
}
125164

165+
// Phase 3: mutate. All checks above passed; this branch is committed.
126166
unset($optionsArray->items[$dialogClassIdx]);
127167
$optionsArray->items = array_values($optionsArray->items);
128168

129-
if ($classesIdx === null) {
169+
if ($classesItem === null) {
170+
// Branch A: no `classes` key existed — add a fresh one.
130171
$optionsArray->items[] = new ArrayItem(
131172
new Array_([
132173
new ArrayItem($dialogClassValue, new String_('ui-dialog')),
133174
]),
134175
new String_('classes')
135176
);
136-
} elseif ($uiDialogIdx !== null) {
137-
// dialogClass deletion shifted the indexes — recompute the classes index.
138-
$classesIdx = $this->findKeyIndex($optionsArray, 'classes');
139-
if ($classesIdx === null) {
140-
return null;
141-
}
142-
$classesItem = $optionsArray->items[$classesIdx];
143-
if (!$classesItem->value instanceof Array_) {
144-
return null;
145-
}
146-
147-
$uiDialogItem = $classesItem->value->items[$uiDialogIdx];
148-
149-
if (!($uiDialogItem->value instanceof String_) || !($dialogClassValue instanceof String_)) {
150-
return null;
151-
}
152-
153-
$uiDialogItem->value = new String_(
154-
$uiDialogItem->value->value.' '.$dialogClassValue->value
155-
);
156-
} else {
157-
$classesIdx = $this->findKeyIndex($optionsArray, 'classes');
158-
if ($classesIdx === null) {
159-
return null;
160-
}
161-
$classesItem = $optionsArray->items[$classesIdx];
162-
if (!$classesItem->value instanceof Array_) {
163-
return null;
164-
}
165-
166-
$classesItem->value->items[] = new ArrayItem(
177+
} elseif ($uiDialogItem !== null && $concatenatedString !== null) {
178+
// Branch B: `classes['ui-dialog']` already present — concatenate.
179+
$uiDialogItem->value = new String_($concatenatedString);
180+
} elseif ($classesInnerArray !== null) {
181+
// Branch C: `classes` exists but has no `ui-dialog` entry — append.
182+
$classesInnerArray->items[] = new ArrayItem(
167183
$dialogClassValue,
168184
new String_('ui-dialog')
169185
);
@@ -189,15 +205,4 @@ private function getResolvedClassName(New_ $node): ?string
189205

190206
return null;
191207
}
192-
193-
private function findKeyIndex(Array_ $array, string $keyName): ?int
194-
{
195-
foreach ($array->items as $idx => $item) {
196-
if ($item->key instanceof String_ && $item->key->value === $keyName) {
197-
return $idx;
198-
}
199-
}
200-
201-
return null;
202-
}
203208
}

‎tests/src/Drupal11/Rector/Deprecation/ReplaceDialogClassOptionRector/fixture/no_change_classes_is_variable.php.inc‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
3+
namespace Drupal\Tests\rector_fixture {
4+
5+
// Merging into an existing `classes` entry requires it to resolve to a
6+
// literal array node. When `classes` is a variable (or any non-Array_
7+
// expression), the rector cannot safely insert into it and must leave
8+
// the array untouched.
9+
function withVariableClasses(array $existingClasses) {
10+
return new \Drupal\Core\Ajax\OpenDialogCommand(
11+
'#my-dialog',
12+
'Title',
13+
'content',
14+
[
15+
'dialogClass' => 'extra-class',
16+
'classes' => $existingClasses,
17+
]
18+
);
19+
}
20+
21+
}
22+
?>

‎tests/src/Drupal11/Rector/Deprecation/ReplaceDialogClassOptionRector/fixture/no_change_duplicate_dialog_class.php.inc‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
3+
namespace Drupal\Tests\rector_fixture {
4+
5+
// Pathological duplicate `dialogClass` keys: PHP de-duplicates at array
6+
// construction (last wins) but the AST keeps both. The rector bails so it
7+
// never partially rewrites — picking one would leave the other in place,
8+
// keeping the deprecated key alive.
9+
function withDuplicateDialogClass() {
10+
return new \Drupal\Core\Ajax\OpenDialogCommand(
11+
'#my-dialog',
12+
'Title',
13+
'content',
14+
[
15+
'dialogClass' => 'first',
16+
'dialogClass' => 'second',
17+
]
18+
);
19+
}
20+
21+
}
22+
?>

‎tests/src/Drupal11/Rector/Deprecation/ReplaceDialogClassOptionRector/fixture/no_change_non_literal_new_value.php.inc‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
<?php
2+
3+
namespace Drupal\Tests\rector_fixture {
4+
5+
// Concatenation path requires BOTH the existing 'ui-dialog' value and the
6+
// new dialogClass value to be string literals. When the new value is a
7+
// variable, the rector must leave the array untouched — partially deleting
8+
// dialogClass without inserting a replacement would silently lose the
9+
// option.
10+
function withNonLiteralDialogClass(string $cssClass) {
11+
return new \Drupal\Core\Ajax\OpenDialogCommand(
12+
'#my-dialog',
13+
'Title',
14+
'content',
15+
[
16+
'dialogClass' => $cssClass,
17+
'classes' => ['ui-dialog' => 'base'],
18+
]
19+
);
20+
}
21+
22+
}
23+
?>

0 commit comments

Comments
 (0)