Skip to content

Commit 48328ed

Browse files
michaelruelastaylorotwell
michaelruelas
and
taylorotwell
authored
Fix serialization of multiple closures on the same line (#120)
* Fix serialization of multiple closures on the same line * Apply style fixes and maintain same-line closure tests * Apply additional style fixes to tests * formatting --------- Co-authored-by: Taylor Otwell <[email protected]>
1 parent 29f1806 commit 48328ed

File tree

2 files changed

+303
-13
lines changed

2 files changed

+303
-13
lines changed

‎src/Support/ReflectionClosure.php‎

Lines changed: 193 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,8 @@ public function getCode()
125125
$isUsingScope = false;
126126
$isUsingThisObject = false;
127127

128+
$candidates = [];
129+
128130
for ($i = 0, $l = count($tokens); $i < $l; $i++) {
129131
$token = $tokens[$i];
130132

@@ -296,7 +298,14 @@ public function getCode()
296298
case '}':
297299
$code .= '}';
298300
if (--$open === 0 && ! $isShortClosure) {
299-
break 3;
301+
$reset = $this->collectCandidate($candidates, $code, $use, $isShortClosure, $isUsingThisObject, $isUsingScope);
302+
$code = $reset['code'];
303+
$state = $reset['state'];
304+
$open = $reset['open'];
305+
$use = $reset['use'];
306+
$isShortClosure = $reset['isShortClosure'];
307+
$isUsingThisObject = $reset['isUsingThisObject'];
308+
$isUsingScope = $reset['isUsingScope'];
300309
} elseif ($inside_structure) {
301310
$inside_structure = ! ($open === $inside_structure_mark);
302311
}
@@ -312,7 +321,15 @@ public function getCode()
312321
case ']':
313322
if ($isShortClosure) {
314323
if ($open === 0) {
315-
break 3;
324+
$reset = $this->collectCandidate($candidates, $code, $use, $isShortClosure, $isUsingThisObject, $isUsingScope);
325+
$code = $reset['code'];
326+
$state = $reset['state'];
327+
$open = $reset['open'];
328+
$use = $reset['use'];
329+
$isShortClosure = $reset['isShortClosure'];
330+
$isUsingThisObject = $reset['isUsingThisObject'];
331+
$isUsingScope = $reset['isUsingScope'];
332+
continue 3;
316333
}
317334
$open--;
318335
}
@@ -321,7 +338,15 @@ public function getCode()
321338
case ',':
322339
case ';':
323340
if ($isShortClosure && $open === 0) {
324-
break 3;
341+
$reset = $this->collectCandidate($candidates, $code, $use, $isShortClosure, $isUsingThisObject, $isUsingScope);
342+
$code = $reset['code'];
343+
$state = $reset['state'];
344+
$open = $reset['open'];
345+
$use = $reset['use'];
346+
$isShortClosure = $reset['isShortClosure'];
347+
$isUsingThisObject = $reset['isUsingThisObject'];
348+
$isUsingScope = $reset['isUsingScope'];
349+
continue 3;
325350
}
326351
$code .= $token[0];
327352
break;
@@ -670,16 +695,6 @@ public function getCode()
670695
}
671696
}
672697

673-
if ($isShortClosure) {
674-
$this->useVariables = $this->getStaticVariables();
675-
} else {
676-
$this->useVariables = empty($use) ? $use : array_intersect_key($this->getStaticVariables(), array_flip($use));
677-
}
678-
679-
$this->isShortClosure = $isShortClosure;
680-
$this->isBindingRequired = $isUsingThisObject;
681-
$this->isScopeRequired = $isUsingScope;
682-
683698
$attributesCode = array_map(function ($attribute) {
684699
$arguments = $attribute->getArguments();
685700

@@ -697,6 +712,36 @@ public function getCode()
697712
return "#[$name($arguments)]";
698713
}, $this->getAttributes());
699714

715+
if (count($candidates) > 1) {
716+
$lastItem = array_pop($candidates);
717+
718+
foreach ($candidates as $candidate) {
719+
if (! $this->verifyCandidateSignature($candidate)) {
720+
continue;
721+
}
722+
723+
$this->applyCandidate($candidate);
724+
725+
$code = $candidate['code'];
726+
727+
if (! empty($attributesCode)) {
728+
$code = implode("\n", array_merge($attributesCode, [$code]));
729+
}
730+
731+
$this->code = $code;
732+
733+
return $this->code;
734+
}
735+
736+
$candidates[] = $lastItem;
737+
}
738+
739+
$lastItem = array_pop($candidates);
740+
741+
$this->applyCandidate($lastItem);
742+
743+
$code = $lastItem['code'];
744+
700745
if (! empty($attributesCode)) {
701746
$code = implode("\n", array_merge($attributesCode, [$code]));
702747
}
@@ -727,6 +772,10 @@ public function getUseVariables()
727772
return $this->useVariables;
728773
}
729774

775+
if ($this->isShortClosure()) {
776+
return $this->useVariables;
777+
}
778+
730779
$tokens = $this->getTokens();
731780
$use = [];
732781
$state = 'start';
@@ -1240,4 +1289,135 @@ protected function parseNameQualified($token)
12401289

12411290
return [$id_start, $id_start_ci, $id_name];
12421291
}
1292+
1293+
/**
1294+
* Collect a closure candidate and reset state for finding the next one.
1295+
*
1296+
* @param array $candidates
1297+
* @param string $code
1298+
* @param array $use
1299+
* @param bool $isShortClosure
1300+
* @param bool $isUsingThisObject
1301+
* @param bool $isUsingScope
1302+
* @return array
1303+
*/
1304+
protected function collectCandidate(&$candidates, $code, $use, $isShortClosure, $isUsingThisObject, $isUsingScope)
1305+
{
1306+
$candidates[] = [
1307+
'code' => $code,
1308+
'use' => $use,
1309+
'isShortClosure' => $isShortClosure,
1310+
'isUsingThisObject' => $isUsingThisObject,
1311+
'isUsingScope' => $isUsingScope,
1312+
];
1313+
1314+
return [
1315+
'code' => '',
1316+
'state' => 'start',
1317+
'open' => 0,
1318+
'use' => [],
1319+
'isShortClosure' => false,
1320+
'isUsingThisObject' => false,
1321+
'isUsingScope' => false,
1322+
];
1323+
}
1324+
1325+
/**
1326+
* Apply a candidate's properties to this instance.
1327+
*
1328+
* @param array $candidate
1329+
* @return void
1330+
*/
1331+
protected function applyCandidate($candidate)
1332+
{
1333+
if ($candidate['isShortClosure']) {
1334+
$this->useVariables = $this->getStaticVariables();
1335+
} else {
1336+
$this->useVariables = empty($candidate['use'])
1337+
? $candidate['use']
1338+
: array_intersect_key($this->getStaticVariables(), array_flip($candidate['use']));
1339+
}
1340+
1341+
$this->isShortClosure = $candidate['isShortClosure'];
1342+
$this->isBindingRequired = $candidate['isUsingThisObject'];
1343+
$this->isScopeRequired = $candidate['isUsingScope'];
1344+
}
1345+
1346+
/**
1347+
* Verify that a candidate matches the closure's signature.
1348+
*
1349+
* @param array $candidate
1350+
* @return bool
1351+
*/
1352+
protected function verifyCandidateSignature($candidate)
1353+
{
1354+
$code = $candidate['code'];
1355+
$use = $candidate['use'];
1356+
$isShortClosure = $candidate['isShortClosure'];
1357+
1358+
// Check if code starts with 'static' (more precise than searching anywhere in code)
1359+
$isStaticCode = strtolower(substr(ltrim($code), 0, 6)) === 'static';
1360+
if (parent::isStatic() !== $isStaticCode) {
1361+
return false;
1362+
}
1363+
1364+
// Parse the candidate to extract parameters and variables
1365+
$tokens = token_get_all('<?php '.$code);
1366+
$params = [];
1367+
$vars = [];
1368+
$state = 'start';
1369+
1370+
foreach ($tokens as $token) {
1371+
if (! is_array($token)) {
1372+
if ($token === '(' && $state === 'start') {
1373+
$state = 'params';
1374+
} elseif ($token === ')' && $state === 'params') {
1375+
$state = 'body';
1376+
}
1377+
1378+
continue;
1379+
}
1380+
1381+
if ($token[0] === T_VARIABLE) {
1382+
$name = substr($token[1], 1);
1383+
1384+
if ($state === 'params') {
1385+
$params[] = $name;
1386+
} elseif ($state === 'body' && $name !== 'this') {
1387+
$vars[$name] = true;
1388+
}
1389+
}
1390+
}
1391+
1392+
// Verify parameter count
1393+
if (parent::getNumberOfParameters() !== count($params)) {
1394+
return false;
1395+
}
1396+
1397+
// Verify use/captured variables
1398+
if ($isShortClosure) {
1399+
$actualVars = array_keys(parent::getStaticVariables());
1400+
$foundCaptures = array_diff(array_keys($vars), $params);
1401+
1402+
if (count($foundCaptures) !== count($actualVars)) {
1403+
return false;
1404+
}
1405+
1406+
if (count(array_diff($foundCaptures, $actualVars)) > 0) {
1407+
return false;
1408+
}
1409+
} else {
1410+
$actualStaticVariables = array_keys(parent::getStaticVariables());
1411+
1412+
if (! empty($use) && count(array_diff($use, $actualStaticVariables)) > 0) {
1413+
return false;
1414+
}
1415+
1416+
if (count($use) !== count(parent::getStaticVariables())) {
1417+
return false;
1418+
}
1419+
}
1420+
1421+
return true;
1422+
}
12431423
}

‎tests/MultipleClosuresOnSameLineTest.php‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
<?php
2+
3+
use Laravel\SerializableClosure\SerializableClosure;
4+
5+
test('multiple closures on same line with different arguments', function () {
6+
$c1 = fn ($a) => $a;
7+
$c2 = fn ($b) => $b; // @phpstan-ignore-line
8+
9+
$s1 = new SerializableClosure($c1);
10+
$s2 = new SerializableClosure($c2);
11+
12+
expect($s1->getClosure()(1))->toBe(1);
13+
expect($s2->getClosure()(2))->toBe(2);
14+
15+
$u1 = unserialize(serialize($s1))->getClosure();
16+
$u2 = unserialize(serialize($s2))->getClosure();
17+
18+
expect($u1(1))->toBe(1);
19+
expect($u2(2))->toBe(2);
20+
});
21+
22+
test('multiple closures on same line with different static variables', function () {
23+
$a = 1;
24+
$b = 2;
25+
$c1 = fn () => $a;
26+
$c2 = fn () => $b; // @phpstan-ignore-line
27+
28+
$s1 = new SerializableClosure($c1);
29+
$s2 = new SerializableClosure($c2);
30+
31+
expect($s1->getClosure()())->toBe(1);
32+
expect($s2->getClosure()())->toBe(2);
33+
34+
$u1 = unserialize(serialize($s1))->getClosure();
35+
$u2 = unserialize(serialize($s2))->getClosure();
36+
37+
expect($u1())->toBe(1);
38+
expect($u2())->toBe(2);
39+
});
40+
41+
test('mixture of static and non-static closures', function () {
42+
$c1 = fn () => 1;
43+
$c2 = static fn () => 2; // @phpstan-ignore-line
44+
45+
$s1 = new SerializableClosure($c1);
46+
$s2 = new SerializableClosure($c2);
47+
48+
expect($s1->getClosure()())->toBe(1);
49+
expect($s2->getClosure()())->toBe(2);
50+
51+
$u1 = unserialize(serialize($s1))->getClosure();
52+
$u2 = unserialize(serialize($s2))->getClosure();
53+
54+
expect($u1())->toBe(1);
55+
expect($u2())->toBe(2);
56+
});
57+
58+
test('closure using variable named static is not detected as static closure', function () {
59+
$static = 'not a static closure';
60+
$c1 = fn () => $static;
61+
$c2 = fn () => 'other'; // @phpstan-ignore-line
62+
63+
$s1 = new SerializableClosure($c1);
64+
$s2 = new SerializableClosure($c2);
65+
66+
expect($s1->getClosure()())->toBe('not a static closure');
67+
expect($s2->getClosure()())->toBe('other');
68+
69+
$u1 = unserialize(serialize($s1))->getClosure();
70+
$u2 = unserialize(serialize($s2))->getClosure();
71+
72+
expect($u1())->toBe('not a static closure');
73+
expect($u2())->toBe('other');
74+
});
75+
76+
test('multiple traditional closures on same line', function () {
77+
$c1 = function () { return 1; };
78+
$c2 = function () { return 2; }; // @phpstan-ignore-line
79+
80+
$s1 = new SerializableClosure($c1);
81+
$s2 = new SerializableClosure($c2);
82+
83+
expect($s1->getClosure()())->toBe(1);
84+
expect($s2->getClosure()())->toBe(2);
85+
86+
$u1 = unserialize(serialize($s1))->getClosure();
87+
$u2 = unserialize(serialize($s2))->getClosure();
88+
89+
expect($u1())->toBe(1);
90+
expect($u2())->toBe(2);
91+
});
92+
93+
test('multiple traditional closures with use clause on same line', function () {
94+
$a = 1;
95+
$b = 2;
96+
$c1 = function () use ($a) { return $a; };
97+
$c2 = function () use ($b) { return $b; }; // @phpstan-ignore-line
98+
99+
$s1 = new SerializableClosure($c1);
100+
$s2 = new SerializableClosure($c2);
101+
102+
expect($s1->getClosure()())->toBe(1);
103+
expect($s2->getClosure()())->toBe(2);
104+
105+
$u1 = unserialize(serialize($s1))->getClosure();
106+
$u2 = unserialize(serialize($s2))->getClosure();
107+
108+
expect($u1())->toBe(1);
109+
expect($u2())->toBe(2);
110+
});

0 commit comments

Comments
 (0)