Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix error when generating urn catalog for empty misc.xml #11686

Merged
34 changes: 15 additions & 19 deletions app/code/Magento/Developer/Model/XmlCatalog/Format/PhpStorm.php
Expand Up @@ -6,11 +6,11 @@

namespace Magento\Developer\Model\XmlCatalog\Format;

use Magento\Developer\Model\XmlCatalog\Format\PhpStorm\DomDocumentFactory;
use Magento\Framework\Exception\FileSystemException;
use Magento\Framework\Filesystem\Directory\ReadFactory;
use Magento\Framework\Filesystem\Directory\ReadInterface;
use Magento\Framework\Filesystem\Directory\WriteFactory;
use Magento\Framework\Filesystem\Directory\WriteInterface;
use Magento\Framework\Filesystem\File\WriteFactory as FileWriteFactory;

/**
* Class PhpStorm generates URN catalog for PhpStorm 9
Expand All @@ -23,20 +23,28 @@ class PhpStorm implements FormatInterface
private $currentDirRead;

/**
* @var \Magento\Framework\Filesystem\File\WriteFactory
* @var FileWriteFactory
*/
private $fileWriteFactory;

/**
* @var DomDocumentFactory
*/
private $domDocumentFactory;

/**
* @param ReadFactory $readFactory
* @param \Magento\Framework\Filesystem\File\WriteFactory $fileWriteFactory
* @param FileWriteFactory $fileWriteFactory
* @param DomDocumentFactory $domDocumentFactory
*/
public function __construct(
ReadFactory $readFactory,
\Magento\Framework\Filesystem\File\WriteFactory $fileWriteFactory
FileWriteFactory $fileWriteFactory,
DomDocumentFactory $domDocumentFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was already wondering how BC was being handled! I'll make the constructor parameter optional for BC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be a good idea to revert the change made to \Magento\Framework\Filesystem\File\WriteFactory $fileWriteFactory as this is not specific to your change and I am not 100% sure how BC would work in this case.

) {
$this->currentDirRead = $readFactory->create(getcwd());
$this->fileWriteFactory = $fileWriteFactory;
$this->domDocumentFactory = $domDocumentFactory;
}

/**
Expand All @@ -57,26 +65,14 @@ public function generateCatalog(array $dictionary, $configFilePath)
\Magento\Framework\Filesystem\DriverPool::FILE,
'r'
);
$dom = new \DOMDocument();
$dom->loadXML($file->readAll());
$dom = $this->domDocumentFactory->create($file->readAll());
$xpath = new \DOMXPath($dom);
$nodeList = $xpath->query('/project');
$projectNode = $nodeList->item(0);
$file->close();
} catch (FileSystemException $f) {
//create file if does not exists
$dom = new \DOMDocument();
$projectNode = $dom->createElement('project');

//PhpStorm 9 version for component is "4"
$projectNode->setAttribute('version', '4');
$dom->appendChild($projectNode);
$rootComponentNode = $dom->createElement('component');

//PhpStorm 9 version for ProjectRootManager is "2"
$rootComponentNode->setAttribute('version', '2');
$rootComponentNode->setAttribute('name', 'ProjectRootManager');
$projectNode->appendChild($rootComponentNode);
$dom = $this->domDocumentFactory->create();
}

$xpath = new \DOMXPath($dom);
Expand Down
@@ -0,0 +1,49 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Developer\Model\XmlCatalog\Format\PhpStorm;

use DOMDocument;

class DomDocumentFactory extends \Magento\Framework\DomDocument\DomDocumentFactory
{
/**
* {@inheritdoc}
*/
public function create($data = null)
{
$dom = parent::create($data);

if (empty($data)) {
$this->initializeDocument($dom);
}

return $dom;
}

/**
* Initialize document to be used as 'misc.xml'
*
* @param DOMDocument $document
* @return DOMDocument
*/
private function initializeDocument(DOMDocument $document)
{
$document->xmlVersion = '1.0';
$projectNode = $document->createElement('project');

//PhpStorm 9 version for component is "4"
$projectNode->setAttribute('version', '4');
$document->appendChild($projectNode);
$rootComponentNode = $document->createElement('component');

//PhpStorm 9 version for ProjectRootManager is "2"
$rootComponentNode->setAttribute('version', '2');
$rootComponentNode->setAttribute('name', 'ProjectRootManager');
$projectNode->appendChild($rootComponentNode);

return $document;
}
}
31 changes: 28 additions & 3 deletions lib/internal/Magento/Framework/DomDocument/DomDocumentFactory.php
Expand Up @@ -3,20 +3,45 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Framework\DomDocument;

use DOMDocument;

/**
* DOM document factory
*/
class DomDocumentFactory
{
/**
* @var \Magento\Framework\ObjectManagerInterface
*/
private $objectManager;

/**
* DomDocumentFactory constructor.
* @param \Magento\Framework\ObjectManagerInterface $objectManager
*/
public function __construct(\Magento\Framework\ObjectManagerInterface $objectManager)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make the changes to this file or are we always using the new Dom factory class? My worry here is that a change like this will break backwards compatibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, this breaks BC. I can make the $objectManager dependency optional?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this is a private class so we could choose to break BC in this case. Otherwise you could also make the object manager a static call. Another thing to note is that the factory should only be concerned with creating the object and not filling it. In this case initializeDocument in the other factory and calling $dom->loadXML($data) load in the other factory is not really the way to go in my mind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. Factories should only instantiate objects. Wouldn't it be better to migrate all the domain specific code back to \Magento\Developer\Model\XmlCatalog\Format\PhpStorm?

Sorry for all the corrections, refactorings, etc. I'm learning a lot from creating Magento 2 pull requests :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. I see two options here.

  1. Migrate all the domain specific code back to the \Magento\Developer\Model\XmlCatalog\Format\PhpStorm.
  2. Create our own classes for the two dom documents that have the public methods that we need. Then change the new factories to instantiate these new objects.

Not sure what you would prefer to do. If you do go with the idea of new classes then please have a look through http://devdocs.magento.com/guides/v2.2/coding-standards/technical-guidelines/technical-guidelines.html#2-class-design and of course let me know if you need some assistance.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdgroot do you think you will have time to look into this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmanners I'm currently too busy to get working on this PR. Next week I hope to find some time.

{
$this->objectManager = $objectManager;
}

/**
* Create empty DOM document instance.
*
* @return \DOMDocument
* @param string $data the data to be loaded into the object
*
* @return DOMDocument
*/
public function create()
public function create($data = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will need to double check if this will break BC also as according to http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/index.html#adding-parameters-in-public-methods you cannot add new parameters to public methods. Though all BC breaks can be approved let me check internally here and get back to you on this one.

{
return new \DOMDocument();
$dom = $this->objectManager->create(DOMDocument::class);

if (!empty($data) && is_string($data)) {
$dom->loadXML($data);
}

return $dom;
}
}
@@ -0,0 +1,95 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Framework\Test\Unit\DomDocument;

use DOMDocument;
use Magento\Framework\DomDocument\DomDocumentFactory;
use Magento\Framework\ObjectManagerInterface;
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper;

class DomDocumentFactoryTest extends \PHPUnit\Framework\TestCase
{
/**
* @var ObjectManagerHelper
*/
private $objectManagerHelper;

/**
* @var ObjectManagerInterface|\PHPUnit_Framework_MockObject_MockObject
*/
private $objectManagerMock;

/**
* @var DOMDocument|\PHPUnit_Framework_MockObject_MockObject
*/
private $domDocumentMock;

/**
* @var DomDocumentFactory
*/
private $domDocumentFactory;

/**
* @var string
*/
private $xmlSample = <<<EOT
<?xml version="1.0"?>
<root>
</root>
EOT;

/**
* {@inheritdoc}
*/
protected function setUp()
{
$this->objectManagerHelper = new ObjectManagerHelper($this);
$this->objectManagerMock = $this->createMock(ObjectManagerInterface::class);
$this->domDocumentMock = $this->createMock(DOMDocument::class);
$this->domDocumentFactory = $this->objectManagerHelper->getObject(
DomDocumentFactory::class,
[
'objectManager' => $this->objectManagerMock
]
);
}

/**
* @dataProvider createDataProvider
*/
public function testCreate($data = null)
{
$this->objectManagerMock->expects($this->once())
->method('create')
->with(DOMDocument::class)
->willReturn($this->domDocumentMock);

if (empty($data) || !is_string($data)) {
$this->domDocumentMock->expects($this->never())
->method('loadXML');
} else {
$this->domDocumentMock->expects($this->once())
->method('loadXML')
->with($data)
->willReturn(true);
}

$this->domDocumentFactory->create($data);
}

/**
* @return array
*/
public function createDataProvider(): array
{
return [
[null],
[''],
[$this->xmlSample]
];
}
}