Skip to content

Commit d2efda3

Browse files
author
Joan He
committed
MAGETWO-62253: Refactor code to remove unserialize
- address code review comments
1 parent 0c5e6ee commit d2efda3

File tree

5 files changed

+137
-64
lines changed

5 files changed

+137
-64
lines changed

‎app/code/Magento/Backend/Model/Menu.php

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,12 @@ class Menu extends \ArrayObject
3939
private $serializer;
4040

4141
/**
42+
* Menu constructor
43+
*
4244
* @param LoggerInterface $logger
4345
* @param string $pathInMenuStructure
44-
* @param Factory $menuItemFactory
45-
* @param SerializerInterface $serializer
46+
* @param Factory|null $menuItemFactory
47+
* @param SerializerInterface|null $serializer
4648
*/
4749
public function __construct(
4850
LoggerInterface $logger,
@@ -258,17 +260,13 @@ protected function _findParentItems($menu, $itemId, &$parents)
258260
}
259261

260262
/**
261-
* Hack to unset logger instance which cannot be serialized
263+
* Serialize menu
262264
*
263265
* @return string
264266
*/
265267
public function serialize()
266268
{
267-
$logger = $this->_logger;
268-
unset($this->_logger);
269-
$result = $this->serializer->serialize($this->toArray());
270-
$this->_logger = $logger;
271-
return $result;
269+
return $this->serializer->serialize($this->toArray());
272270
}
273271

274272
/**

‎app/code/Magento/Backend/Test/Unit/Model/Menu/Filter/IteratorTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,12 +12,12 @@ class IteratorTest extends \PHPUnit_Framework_TestCase
1212
/**
1313
* @var \Magento\Backend\Model\Menu
1414
*/
15-
protected $menuModel;
15+
private $menuModel;
1616

1717
/**
1818
* @var \Magento\Backend\Model\Menu\Item[]
1919
*/
20-
protected $items = [];
20+
private $items = [];
2121

2222
protected function setUp()
2323
{

‎app/code/Magento/Backend/Test/Unit/Model/Menu/ItemTest.php

Lines changed: 112 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ public function testGetChildrenCreatesSubmenuOnFirstCall()
226226
* @param array $expected
227227
* @dataProvider toArrayDataProvider
228228
*/
229-
public function testToArray($data, $expected)
229+
public function testToArray(array $data, array $expected)
230230
{
231231
$menuMock = $this->getMock(\Magento\Backend\Model\Menu::class, [], [], '', false);
232232
$this->_menuFactoryMock->method('create')->will($this->returnValue($menuMock));
@@ -251,8 +251,16 @@ public function testToArray($data, $expected)
251251
public function toArrayDataProvider()
252252
{
253253
return [
254-
[
255-
$this->_params,
254+
'No submenu' => [
255+
[
256+
'id' => 'item',
257+
'title' => 'Item Title',
258+
'action' => '/system/config',
259+
'resource' => 'Magento_Config::config',
260+
'depends_on_module' => 'Magento_Backend',
261+
'depends_on_config' => 'system/config/isEnabled',
262+
'tooltip' => 'Item tooltip',
263+
],
256264
[
257265
'parent_id' => null,
258266
'module_name' => 'Magento_Backend',
@@ -268,7 +276,7 @@ public function toArrayDataProvider()
268276
'sub_menu' => null
269277
]
270278
],
271-
[
279+
'with submenu' => [
272280
[
273281
'parent_id' => '1',
274282
'module_name' => 'Magento_Module1',
@@ -281,7 +289,15 @@ public function toArrayDataProvider()
281289
'depends_on_module' => null,
282290
'tooltip' => null,
283291
'title' => null,
284-
'sub_menu' => $this->_params,
292+
'sub_menu' => [
293+
'id' => 'item',
294+
'title' => 'Item Title',
295+
'action' => '/system/config',
296+
'resource' => 'Magento_Config::config',
297+
'depends_on_module' => 'Magento_Backend',
298+
'depends_on_config' => 'system/config/isEnabled',
299+
'tooltip' => 'Item tooltip',
300+
],
285301
],
286302
[
287303
'parent_id' => '1',
@@ -295,15 +311,31 @@ public function toArrayDataProvider()
295311
'depends_on_module' => null,
296312
'tooltip' => '',
297313
'title' => null,
298-
'sub_menu' => $this->_params
314+
'sub_menu' => [
315+
'id' => 'item',
316+
'title' => 'Item Title',
317+
'action' => '/system/config',
318+
'resource' => 'Magento_Config::config',
319+
'depends_on_module' => 'Magento_Backend',
320+
'depends_on_config' => 'system/config/isEnabled',
321+
'tooltip' => 'Item tooltip',
322+
]
299323
]
300324
],
301-
[
325+
'small set of data' => [
302326
[
303327
'parent_id' => '1',
304328
'module_name' => 'Magento_Module1',
305329
'sort_index' => '50',
306-
'sub_menu' => $this->_params,
330+
'sub_menu' => [
331+
'id' => 'item',
332+
'title' => 'Item Title',
333+
'action' => '/system/config',
334+
'resource' => 'Magento_Config::config',
335+
'depends_on_module' => 'Magento_Backend',
336+
'depends_on_config' => 'system/config/isEnabled',
337+
'tooltip' => 'Item tooltip',
338+
],
307339
],
308340
[
309341
'parent_id' => '1',
@@ -317,7 +349,15 @@ public function toArrayDataProvider()
317349
'depends_on_module' => null,
318350
'tooltip' => '',
319351
'title' => null,
320-
'sub_menu' => $this->_params
352+
'sub_menu' => [
353+
'id' => 'item',
354+
'title' => 'Item Title',
355+
'action' => '/system/config',
356+
'resource' => 'Magento_Config::config',
357+
'depends_on_module' => 'Magento_Backend',
358+
'depends_on_config' => 'system/config/isEnabled',
359+
'tooltip' => 'Item tooltip',
360+
]
321361
]
322362
]
323363
];
@@ -326,10 +366,14 @@ public function toArrayDataProvider()
326366
/**
327367
* @param array $constructorData
328368
* @param array $populateFromData
369+
* @param array $expected
329370
* @dataProvider populateFromArrayDataProvider
330371
*/
331-
public function testPopulateFromArray($constructorData, $populateFromData, $expected)
332-
{
372+
public function testPopulateFromArray(
373+
array $constructorData,
374+
array $populateFromData,
375+
array $expected
376+
) {
333377
$menuMock = $this->getMock(\Magento\Backend\Model\Menu::class, [], [], '', false);
334378
$this->_menuFactoryMock->method('create')->will($this->returnValue($menuMock));
335379
$menuMock->method('toArray')
@@ -354,9 +398,17 @@ public function testPopulateFromArray($constructorData, $populateFromData, $expe
354398
public function populateFromArrayDataProvider()
355399
{
356400
return [
357-
[
401+
'default data to constructor' => [
358402
[],
359-
$this->_params,
403+
[
404+
'id' => 'item',
405+
'title' => 'Item Title',
406+
'action' => '/system/config',
407+
'resource' => 'Magento_Config::config',
408+
'depends_on_module' => 'Magento_Backend',
409+
'depends_on_config' => 'system/config/isEnabled',
410+
'tooltip' => 'Item tooltip',
411+
],
360412
[
361413
'parent_id' => null,
362414
'module_name' => 'Magento_Backend',
@@ -372,8 +424,16 @@ public function populateFromArrayDataProvider()
372424
'sub_menu' => null
373425
],
374426
],
375-
[
376-
$this->_params,
427+
'data without submenu to constructor' => [
428+
[
429+
'id' => 'item',
430+
'title' => 'Item Title',
431+
'action' => '/system/config',
432+
'resource' => 'Magento_Config::config',
433+
'depends_on_module' => 'Magento_Backend',
434+
'depends_on_config' => 'system/config/isEnabled',
435+
'tooltip' => 'Item tooltip',
436+
],
377437
[
378438
'parent_id' => '1',
379439
'module_name' => 'Magento_Module1',
@@ -386,7 +446,15 @@ public function populateFromArrayDataProvider()
386446
'depends_on_module' => null,
387447
'tooltip' => null,
388448
'title' => null,
389-
'sub_menu' => $this->_params,
449+
'sub_menu' => [
450+
'id' => 'item',
451+
'title' => 'Item Title',
452+
'action' => '/system/config',
453+
'resource' => 'Magento_Config::config',
454+
'depends_on_module' => 'Magento_Backend',
455+
'depends_on_config' => 'system/config/isEnabled',
456+
'tooltip' => 'Item tooltip',
457+
],
390458
],
391459
[
392460
'parent_id' => '1',
@@ -403,7 +471,7 @@ public function populateFromArrayDataProvider()
403471
'sub_menu' => null
404472
],
405473
],
406-
[
474+
'data with submenu to constructor' => [
407475
[
408476
'parent_id' => '1',
409477
'module_name' => 'Magento_Module1',
@@ -416,13 +484,29 @@ public function populateFromArrayDataProvider()
416484
'depends_on_module' => null,
417485
'tooltip' => null,
418486
'title' => null,
419-
'sub_menu' => $this->_params,
487+
'sub_menu' => [
488+
'id' => 'item',
489+
'title' => 'Item Title',
490+
'action' => '/system/config',
491+
'resource' => 'Magento_Config::config',
492+
'depends_on_module' => 'Magento_Backend',
493+
'depends_on_config' => 'system/config/isEnabled',
494+
'tooltip' => 'Item tooltip',
495+
],
420496
],
421497
[
422498
'parent_id' => '1',
423499
'module_name' => 'Magento_Module1',
424500
'sort_index' => '50',
425-
'sub_menu' => $this->_params,
501+
'sub_menu' => [
502+
'id' => 'item',
503+
'title' => 'Item Title',
504+
'action' => '/system/config',
505+
'resource' => 'Magento_Config::config',
506+
'depends_on_module' => 'Magento_Backend',
507+
'depends_on_config' => 'system/config/isEnabled',
508+
'tooltip' => 'Item tooltip',
509+
],
426510
],
427511
[
428512
'parent_id' => '1',
@@ -436,7 +520,15 @@ public function populateFromArrayDataProvider()
436520
'depends_on_module' => null,
437521
'tooltip' => '',
438522
'title' => null,
439-
'sub_menu' => $this->_params
523+
'sub_menu' => [
524+
'id' => 'item',
525+
'title' => 'Item Title',
526+
'action' => '/system/config',
527+
'resource' => 'Magento_Config::config',
528+
'depends_on_module' => 'Magento_Backend',
529+
'depends_on_config' => 'system/config/isEnabled',
530+
'tooltip' => 'Item tooltip',
531+
]
440532
],
441533
]
442534
];

‎app/code/Magento/Backend/Test/Unit/Model/MenuTest.php

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -338,10 +338,6 @@ public function testNestedLoop()
338338
$this->assertEquals($expected, $actual);
339339
}
340340

341-
/**
342-
* @covers \Magento\Backend\Model\Menu::toArray
343-
* @covers \Magento\Backend\Model\Menu::serialize
344-
*/
345341
public function testSerialize()
346342
{
347343
$serializerMock = $this->getMock(SerializerInterface::class);
@@ -365,10 +361,6 @@ public function testSerialize()
365361
$this->assertEquals('serializedString', $menu->serialize());
366362
}
367363

368-
/**
369-
* @covers \Magento\Backend\Model\Menu::populateFromArray
370-
* @covers \Magento\Backend\Model\Menu::unserialize
371-
*/
372364
public function testUnserialize()
373365
{
374366
$serializerMock = $this->getMock(SerializerInterface::class);

‎dev/tests/integration/testsuite/Magento/Backend/Model/MenuTest.php

Lines changed: 17 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -9,37 +9,34 @@
99
* Test class for \Magento\Backend\Model\Auth.
1010
*
1111
* @magentoAppArea adminhtml
12-
* @magentoCache all disabled
1312
*/
1413
class MenuTest extends \PHPUnit_Framework_TestCase
1514
{
1615
/**
1716
* @var \Magento\Backend\Model\Menu
1817
*/
19-
protected $_model;
18+
private $model;
19+
20+
/** @var \Magento\Framework\ObjectManagerInterface */
21+
private $objectManager;
2022

2123
protected function setUp()
2224
{
2325
parent::setUp();
2426
\Magento\TestFramework\Helper\Bootstrap::getInstance()
2527
->loadArea(\Magento\Backend\App\Area\FrontNameResolver::AREA_CODE);
26-
$this->_model = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()
27-
->create(\Magento\Backend\Model\Auth::class);
28-
\Magento\TestFramework\Helper\Bootstrap::getObjectManager()->get(
29-
\Magento\Framework\Config\ScopeInterface::class
30-
)->setCurrentScope(\Magento\Backend\App\Area\FrontNameResolver::AREA_CODE);
28+
$this->objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();
29+
$this->model = $this->objectManager->create(\Magento\Backend\Model\Auth::class);
30+
$this->objectManager->get(\Magento\Framework\Config\ScopeInterface::class)
31+
->setCurrentScope(\Magento\Backend\App\Area\FrontNameResolver::AREA_CODE);
3132
}
3233

3334
public function testMenuItemManipulation()
3435
{
3536
/* @var $menu \Magento\Backend\Model\Menu */
36-
$menu = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->create(
37-
\Magento\Backend\Model\Menu\Config::class
38-
)->getMenu();
37+
$menu = $this->objectManager->create(\Magento\Backend\Model\Menu\Config::class)->getMenu();
3938
/* @var $itemFactory \Magento\Backend\Model\Menu\Item\Factory */
40-
$itemFactory = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->create(
41-
\Magento\Backend\Model\Menu\Item\Factory::class
42-
);
39+
$itemFactory = $this->objectManager->create(\Magento\Backend\Model\Menu\Item\Factory::class);
4340

4441
// Add new item in top level
4542
$menu->add(
@@ -84,13 +81,9 @@ public function testMenuItemManipulation()
8481
public function testSerializeUnserialize()
8582
{
8683
/** @var Menu $menu */
87-
$menu = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->get(
88-
\Magento\Backend\Model\MenuFactory::class
89-
)->create();
84+
$menu = $this->objectManager->get(\Magento\Backend\Model\MenuFactory::class)->create();
9085
/* @var \Magento\Backend\Model\Menu\Item\Factory $itemFactory */
91-
$itemFactory = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->create(
92-
\Magento\Backend\Model\Menu\Item\Factory::class
93-
);
86+
$itemFactory = $this->objectManager->create(\Magento\Backend\Model\Menu\Item\Factory::class);
9487

9588
// Add new item in top level
9689
$menu->add(
@@ -104,7 +97,7 @@ public function testSerializeUnserialize()
10497
)
10598
);
10699

107-
//Add submenu
100+
// Add submenu
108101
$menu->add(
109102
$itemFactory->create(
110103
[
@@ -118,11 +111,9 @@ public function testSerializeUnserialize()
118111
'Magento_Backend::system3'
119112
);
120113
$serializedString = $menu->serialize();
121-
/** @var Menu $menu2 */
122-
$menu2 = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->get(
123-
\Magento\Backend\Model\MenuFactory::class
124-
)->create();
125-
$menu2->unserialize($serializedString);
126-
$this->assertEquals($menu, $menu2);
114+
/** @var Menu $unserializedMenu */
115+
$unserializedMenu = $this->objectManager->get(\Magento\Backend\Model\MenuFactory::class)->create();
116+
$unserializedMenu->unserialize($serializedString);
117+
$this->assertEquals($menu, $unserializedMenu);
127118
}
128119
}

0 commit comments

Comments
 (0)