Skip to content

Commit 9e81735

Browse files
KorvinSzantoKorvinSzanto
authored
Unset scopes from transformers after transforming to avoid cyclic reference (#582)
1 parent 927243b commit 9e81735

4 files changed

Lines changed: 167 additions & 41 deletions

File tree

‎src/Scope.php‎

Lines changed: 41 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -264,13 +264,17 @@ public function transformPrimitiveResource()
264264
$transformer = $this->resource->getTransformer();
265265
$data = $this->resource->getData();
266266

267-
if (null === $transformer) {
268-
$transformedData = $data;
269-
} elseif (is_callable($transformer)) {
270-
$transformedData = call_user_func($transformer, $data);
271-
} else {
272-
$transformer->setCurrentScope($this);
273-
$transformedData = $transformer->transform($data);
267+
$this->setScope($transformer);
268+
try {
269+
if (null === $transformer) {
270+
$transformedData = $data;
271+
} elseif (is_callable($transformer)) {
272+
$transformedData = call_user_func($transformer, $data);
273+
} else {
274+
$transformedData = $transformer->transform($data);
275+
}
276+
} finally {
277+
$this->unsetScope($transformer);
274278
}
275279

276280
return $transformedData;
@@ -350,21 +354,24 @@ protected function fireTransformer($transformer, $data): array
350354
{
351355
$includedData = [];
352356

353-
if (is_callable($transformer)) {
354-
$transformedData = call_user_func($transformer, $data);
355-
} else {
356-
$transformer->setCurrentScope($this);
357-
$transformedData = $transformer->transform($data);
358-
}
359-
360-
if ($this->transformerHasIncludes($transformer)) {
361-
$includedData = $this->fireIncludedTransformers($transformer, $data);
362-
$transformedData = $this->manager->getSerializer()->mergeIncludes($transformedData, $includedData);
363-
}
357+
$this->setScope($transformer);
358+
try {
359+
if (is_callable($transformer)) {
360+
$transformedData = call_user_func($transformer, $data);
361+
} else {
362+
$transformedData = $transformer->transform($data);
363+
}
364364

365-
//Stick only with requested fields
366-
$transformedData = $this->filterFieldsets($transformedData);
365+
if ($this->transformerHasIncludes($transformer)) {
366+
$includedData = $this->fireIncludedTransformers($transformer, $data);
367+
$transformedData = $this->manager->getSerializer()->mergeIncludes($transformedData, $includedData);
368+
}
367369

370+
//Stick only with requested fields
371+
$transformedData = $this->filterFieldsets($transformedData);
372+
} finally {
373+
$this->unsetScope($transformer);
374+
}
368375
return [$transformedData, $includedData];
369376
}
370377

@@ -462,4 +469,18 @@ protected function getResourceType(): string
462469
{
463470
return $this->resource->getResourceKey();
464471
}
472+
473+
protected function setScope($transformer): void
474+
{
475+
if ($transformer instanceof TransformerAbstract) {
476+
$transformer->setCurrentScope($this);
477+
}
478+
}
479+
480+
protected function unsetScope($transformer): void
481+
{
482+
if ($transformer instanceof TransformerAbstract) {
483+
$transformer->unsetCurrentScope();
484+
}
485+
}
465486
}

‎src/TransformerAbstract.php‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,13 @@ public function setCurrentScope(Scope $currentScope): self
219219
return $this;
220220
}
221221

222+
public function unsetCurrentScope(): self
223+
{
224+
$this->currentScope = null;
225+
226+
return $this;
227+
}
228+
222229
/**
223230
* Create a new primitive resource object.
224231
*

‎test/MemoryTest.php‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
2+
3+
namespace League\Fractal\Test;
4+
5+
use League\Fractal\Manager;
6+
use League\Fractal\Resource\Collection;
7+
use League\Fractal\Resource\Item;
8+
use League\Fractal\Resource\Primitive;
9+
use League\Fractal\Resource\ResourceInterface;
10+
use League\Fractal\Scope;
11+
use League\Fractal\ScopeFactoryInterface;
12+
use League\Fractal\TransformerAbstract;
13+
use PHPUnit\Framework\TestCase;
14+
15+
class MemoryTest extends TestCase
16+
{
17+
/**
18+
* @dataProvider TransformerProvider
19+
*/
20+
public function testDoesntLeak(ResourceInterface $resource, TransformerAbstract $transformer): void
21+
{
22+
$resource->setTransformer($transformer);
23+
$scopeFactory = \Mockery::mock(ScopeFactoryInterface::class);
24+
$manager = new Manager($scopeFactory);
25+
$scope = new Scope($manager, $resource, 'main');
26+
$subScope = new Scope($manager, new Collection([]), 'sub');
27+
$scopeFactory->shouldReceive('createScopeFor')->andReturn($scope);
28+
$scopeFactory->shouldReceive('createChildScopeFor')->andReturn($subScope);
29+
30+
if ($resource instanceof Primitive) {
31+
$manager->createData($resource)->transformPrimitiveResource();
32+
} else {
33+
$manager->createData($resource)->toArray();
34+
}
35+
$this->assertEquals($scope, $transformer->givenScope ?? null);
36+
37+
if ($transformer->getDefaultIncludes() !== []) {
38+
$this->assertEquals($resource instanceof Primitive ? null : $scope, $transformer->givenSubScope ?? null);
39+
}
40+
$this->assertNull($transformer->getCurrentScope());
41+
}
42+
43+
public function TransformerProvider(): \Generator
44+
{
45+
$basicTransformer = function () {
46+
return new class extends TransformerAbstract {
47+
public $givenScope = null;
48+
public $givenSubScope = null;
49+
50+
public function transform($data)
51+
{
52+
$this->givenScope = $this->getCurrentScope();
53+
return $data;
54+
}
55+
56+
public function includeFoo(): ResourceInterface
57+
{
58+
$this->givenSubScope = $this->getCurrentScope();
59+
return new Item(['foo'], $this);
60+
}
61+
};
62+
};
63+
$resources = [
64+
new Primitive('foo'),
65+
new Collection([['a' => 'b'], ['c' => 'd']]),
66+
new Item(['foo', 'bar', 'baz']),
67+
];
68+
69+
foreach ($resources as $resource) {
70+
yield [$resource, $basicTransformer()];
71+
}
72+
73+
foreach ($resources as $resource) {
74+
$transformer = $basicTransformer();
75+
$transformer->setDefaultIncludes(['foo']);
76+
yield [$resource, $transformer];
77+
}
78+
}
79+
80+
public function testScopeHasNoStrongReferences(): void
81+
{
82+
$transformer = new class extends TransformerAbstract {
83+
public \WeakReference $givenScope;
84+
public function transform($data)
85+
{
86+
$this->givenScope = \WeakReference::create($this->getCurrentScope());
87+
return $data;
88+
}
89+
};
90+
91+
$resource = new Item(['foo', 'bar', 'baz'], $transformer);
92+
$manager = new Manager();
93+
94+
$manager->createData($resource)->toArray();
95+
96+
gc_collect_cycles();
97+
$this->assertNull($transformer->getCurrentScope());
98+
$this->assertNull($transformer->givenScope->get());
99+
}
100+
101+
}

‎test/ScopeTest.php‎

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,17 @@
1515
use League\Fractal\Test\Stub\Transformer\DefaultIncludeBookTransformer;
1616
use League\Fractal\Test\Stub\Transformer\NullIncludeBookTransformer;
1717
use League\Fractal\Test\Stub\Transformer\PrimitiveIncludeBookTransformer;
18+
use League\Fractal\TransformerAbstract;
1819
use Mockery;
19-
use PHPUnit\Framework\TestCase;
20+
use Mockery\Adapter\Phpunit\MockeryTestCase;
2021

21-
class ScopeTest extends TestCase
22+
class ScopeTest extends MockeryTestCase
2223
{
2324
protected $simpleItem = ['foo' => 'bar'];
2425
protected $simpleCollection = [['foo' => 'bar']];
2526

27+
protected TransformerAbstract $transformer;
28+
2629
public function testEmbedChildScope()
2730
{
2831
$manager = new Manager();
@@ -325,9 +328,8 @@ public function testRunAppropriateTransformerWithPrimitive()
325328
{
326329
$manager = new Manager();
327330

328-
$transformer = Mockery::mock('League\Fractal\TransformerAbstract');
329-
$transformer->shouldReceive('transform')->once()->andReturn('simple string');
330-
$transformer->shouldReceive('setCurrentScope')->once()->andReturnSelf();
331+
$this->transformer->return = 'simple string';
332+
$transformer = Mockery::spy($this->transformer);
331333
$transformer->shouldNotReceive('getAvailableIncludes');
332334
$transformer->shouldNotReceive('getDefaultIncludes');
333335

@@ -346,13 +348,7 @@ public function testRunAppropriateTransformerWithItem()
346348
{
347349
$manager = new Manager();
348350

349-
$transformer = Mockery::mock('League\Fractal\TransformerAbstract');
350-
$transformer->shouldReceive('transform')->once()->andReturn($this->simpleItem);
351-
$transformer->shouldReceive('getAvailableIncludes')->once()->andReturn([]);
352-
$transformer->shouldReceive('getDefaultIncludes')->once()->andReturn([]);
353-
$transformer->shouldReceive('setCurrentScope')->once()->andReturnSelf();
354-
355-
$resource = new Item($this->simpleItem, $transformer);
351+
$resource = new Item($this->simpleItem, $this->transformer);
356352
$scope = $manager->createData($resource);
357353

358354
$this->assertSame(['data' => $this->simpleItem], $scope->toArray());
@@ -362,13 +358,7 @@ public function testRunAppropriateTransformerWithCollection()
362358
{
363359
$manager = new Manager();
364360

365-
$transformer = Mockery::mock('League\Fractal\TransformerAbstract');
366-
$transformer->shouldReceive('transform')->once()->andReturn(['foo' => 'bar']);
367-
$transformer->shouldReceive('getAvailableIncludes')->once()->andReturn([]);
368-
$transformer->shouldReceive('getDefaultIncludes')->once()->andReturn([]);
369-
$transformer->shouldReceive('setCurrentScope')->once()->andReturnSelf();
370-
371-
$resource = new Collection([['foo' => 'bar']], $transformer);
361+
$resource = new Collection([['foo' => 'bar']], $this->transformer);
372362
$scope = $manager->createData($resource);
373363

374364
$this->assertSame(['data' => [['foo' => 'bar']]], $scope->toArray());
@@ -722,9 +712,16 @@ public function fieldsetsWithSideLoadIncludesProvider()
722712
];
723713
}
724714

725-
public function tearDown(): void
715+
public function setUp(): void
726716
{
727-
Mockery::close();
717+
$this->transformer = new class extends TransformerAbstract {
718+
public const RETURN_DATA = 'eeb2009e-a16e-4bcf-8ce7-e4782a520515';
719+
public $return = self::RETURN_DATA;
720+
public function transform($data)
721+
{
722+
return $this->return === self::RETURN_DATA ? $data : $this->return;
723+
}
724+
};
728725
}
729726

730727
protected function createTransformerWithIncludedResource($resourceName, $transformResult)

0 commit comments

Comments
 (0)