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

tdgroot · 2017-10-24T12:20:34Z

Fix error when generating urn catalog for empty misc.xml

Description

Fix error when generating urn catalog for existing empty misc.xml. I fixed this by creating another DomDocumentFactory, for PhpStorm specific misc.xml. This factory checks if the passed data is empty, and if so, create the misc.xml xml structure that \Magento\Developer\Model\XmlCatalog\Format\PhpStorm expects.

I also changed \Magento\Framework\DomDocument\DomDocumentFactory to follow the Magento 2 guidelines and added a Test class for this factory to define the behavior.

Fixed Issues (if relevant)

Error generating URN-catalog when blank one exists #5188

Manual testing scenarios

rm .idea/misc..xml
touch .idea/misc.xml
bin/magento dev:urn-catalog:generate .idea/misc.xml

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

tdgroot · 2017-10-25T06:56:37Z

Codacy is not successful, because I used an } else { statement. Any suggestions?

dmanners · 2017-11-01T10:28:26Z

@tdgroot thanks for the PR. I will start to process this now. Do not worry about the codacy check for the else that result seems a bit odd to me.

dmanners

At least 1 change to be made for BC reasons. I will get back to you about the changes to the factory class.

dmanners · 2017-11-01T10:34:14Z

app/code/Magento/Developer/Model/XmlCatalog/Format/PhpStorm.php

     */
    public function __construct(
        ReadFactory $readFactory,
-        \Magento\Framework\Filesystem\File\WriteFactory $fileWriteFactory
+        FileWriteFactory $fileWriteFactory,
+        DomDocumentFactory $domDocumentFactory


This change will break BC try adding an optional dependency here as described in http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/index.html#adding-a-constructor-parameter

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

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.

dmanners · 2017-11-01T10:41:22Z

lib/internal/Magento/Framework/DomDocument/DomDocumentFactory.php

     */
-    public function create()
+    public function create($data = null)


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.

dmanners · 2017-11-01T12:56:57Z

Ok @tdgroot here is what I am thinking so that we do not break BC and then this can be merged into 2.2.

It is not recommended to use inheritance inside of Magento2 but rather composition. So what I would suggest we do here is update app/code/Magento/Developer/Model/XmlCatalog/Format/PhpStorm/DomDocumentFactory.php so that it does not extends the lib/internal/Magento/Framework/DomDocument/DomDocumentFactory.php.
Revert the changes to lib/internal/Magento/Framework/DomDocument/DomDocumentFactory.php as I am not sure how we can make these without breaking BC.
Add all the required methods into the new factory so that it will work as expected.

After this has been done and merged we could consider making the more detailed change into the 2.3 release in which case we can make a few more changes with regards to BC.

How does that sound to you?

tdgroot · 2017-11-07T10:07:53Z

@dmanners thank you for your feedback.

They seem fair points, I'll need to find some time coming week to propose the changes based on your feedback. I have one question, though.

How did the changes in lib/internal/Magento/Framework/DomDocument/DomDocumentFactory.php break BC? The $data parameter is optional, so it should work fine for piece of code that uses the method. Or is it just against Magento's guidelines?

dmanners · 2017-11-09T13:01:02Z

@tdgroot yes sorry that was my mistake I missed the fact it was optional. That will be fine.

tdgroot · 2017-11-11T11:01:49Z

Hi @dmanners. I added BC to the code and tweaked some parts as well!

dmanners · 2017-11-13T15:27:22Z

lib/internal/Magento/Framework/DomDocument/DomDocumentFactory.php

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


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.

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

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.

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 :).

No worries. I see two options here.

Migrate all the domain specific code back to the \Magento\Developer\Model\XmlCatalog\Format\PhpStorm.

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.

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

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

dmanners · 2017-12-20T09:55:58Z

Hi @tdgroot if you would like help with this pull request I would be happy to take over and see if we can get this sorted before the end of 2018. How does that sound?

tdgroot · 2017-12-20T10:19:56Z

@dmanners Sure! Rounding up 2017 is getting really busy unfortunately..

…e phpstorm specific file

…rns the expected type of object

#11686

tdgroot added 2 commits October 24, 2017 12:39

Fix error when generating urn catalog for empty misc.xml

177e540

Add unit test for \Magento\Framework\DomDocument\DomDocumentFactory

0d65213

dmanners self-assigned this Nov 1, 2017

dmanners added this to the November 2017 milestone Nov 1, 2017

dmanners added Release Line: 2.2 2.2.x Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 1, 2017

dmanners suggested changes Nov 1, 2017

View reviewed changes

magento-team added the Progress: needs update label Nov 1, 2017

magento-team removed the Progress: needs update label Nov 9, 2017

magento-team added the Progress: needs update label Nov 9, 2017

Add backwards compatibility to XmlCatalog PhpStorm changes

a781bf6

magento-team removed the Progress: needs update label Nov 13, 2017

dmanners reviewed Nov 13, 2017

View reviewed changes

magento-team added the Progress: needs update label Nov 14, 2017

okorshenko modified the milestones: November 2017, December 2017 Dec 1, 2017

okorshenko removed the 2.2.x label Dec 14, 2017

magento-team removed the Progress: needs update label Dec 20, 2017

dmanners added 4 commits December 20, 2017 11:29

magento#5188: Revert the loading of xml from the factory back into th…

dcc480d

…e phpstorm specific file

magento#5188: Refactor the basic file setup into it's own method

5d4e1c2

magento#5188: Remove the no longer needed xml catalog domfactory class

b552dd1

magento#5188: Update DomDocumentFactoryTest to check that create retu…

b85b91e

…rns the expected type of object

dmanners approved these changes Dec 20, 2017

View reviewed changes

magento-team added the Progress: accept label Dec 21, 2017

okorshenko mentioned this pull request Dec 22, 2017

Error generating URN-catalog when blank one exists #5188

Closed

magento-team merged commit b85b91e into magento:2.2-develop Dec 22, 2017

magento-team pushed a commit that referenced this pull request Dec 22, 2017

MAGETWO-83120: Fix error when generating urn catalog for empty misc.xml

3563c25

#11686

magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Dec 22, 2017

tdgroot deleted the 2.2-develop-fix_empty_idea_misc_xml branch June 2, 2018 15:57

gelanivishal mentioned this pull request Jul 18, 2018

[Backport] Fix error when generating urn catalog for empty misc.xml #16902

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tdgroot commented Oct 24, 2017

tdgroot commented Oct 25, 2017

dmanners commented Nov 1, 2017

dmanners left a comment

dmanners Nov 1, 2017

tdgroot Nov 1, 2017

dmanners Nov 1, 2017

dmanners Nov 1, 2017

dmanners commented Nov 1, 2017

tdgroot commented Nov 7, 2017

dmanners commented Nov 9, 2017

tdgroot commented Nov 11, 2017

dmanners Nov 13, 2017

tdgroot Nov 13, 2017

dmanners Nov 13, 2017

tdgroot Nov 20, 2017

dmanners Nov 21, 2017

dmanners Nov 28, 2017

tdgroot Nov 28, 2017

dmanners commented Dec 20, 2017

tdgroot commented Dec 20, 2017

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

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

Conversation

tdgroot commented Oct 24, 2017

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

tdgroot commented Oct 25, 2017

dmanners commented Nov 1, 2017

dmanners left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmanners commented Nov 1, 2017

tdgroot commented Nov 7, 2017

dmanners commented Nov 9, 2017

tdgroot commented Nov 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmanners commented Dec 20, 2017

tdgroot commented Dec 20, 2017