add more CDATA-related tests for Magento\Framework\Config\Dom::merge and fix failing ones
#18336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…` and create a fix for failing ones
|
|
|
The build is failing with quite strange message: |
|
I closed and re-opened PR just to trigger new build on Travis |
aleron75
approved these changes
Oct 8, 2018
There was a problem hiding this comment.
Good work! Thanks for your contribution!
Once merged, don't forget you can easily port this PR to 2.2 branch with the Porting Tool, read more here.
|
Hi @aleron75, thank you for the review. |
sidolov
requested changes
Oct 8, 2018
| <?xml version="1.0"?> | ||
| <!-- | ||
| Copyright (c) Vaimo Group. All rights reserved. | ||
| See LICENSE_VAIMO.txt for license details. |
There was a problem hiding this comment.
Please, replace comments with corresponded Magento copyrights
|
Hello @sidolov! I fixed copyright comments in all created files but now build is failing on automated code review stage. And the error there is quite interesting: Basically, it complains about method naming. I absolutely agree that the methods I've created should be private and camelCase but... There is one "BUT" here :) All other methods in this class have What should I do? Break naming consistency for this class or ignore code review issues? |
not sure so I ask confirmation to @sidolov: maybe in this case we may add an annotation to ignore this kind of static analysis complaints? |
|
Hi @enl, |
|
Hi @enl , changed files contain legacy code and old coding standards, you should try to avoid underscores in methods and properties names. All non-public methods should be private according to our rules that defined in Technical Guidelines in section Class design. |
|
Hi @sidolov any news on this? |
sidolov
approved these changes
Oct 23, 2018
|
Hi @sidolov, thank you for the review. |
|
Hi @enl , we tried to reproduce described issue on the 2.3-develop branch and looks like it works as expected. Steps to reproduce:
Maybe we missed something, please, add additional info to reproduce it. |
|
Hello @sidolov! I've updated my fork to get all the changes from upstream and then performed the following steps:
<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:module:Magento_Config:etc/system_file.xsd">
<system>
<section id="system">
<group id="full_page_cache">
<group id="fastly" translate="label" showInDefault="1" showInWebsite="0" showInStore="0" sortOrder="615">
<group id="fastly_advanced_configuration" sortOrder="50" translate="label comment" showInDefault="1" showInWebsite="0" showInStore="0">
<field id="geoip_action" translate="label comment" type="select" sortOrder="70" showInDefault="1" showInWebsite="0" showInStore="0">
<label>GeoIP Action</label>
<comment>
<![CDATA["Dialog" shows a modal dialog to select target store.<br /> "Redirect" redirects the client directly to the target store. <br />Hello, world!]]>
</comment>
<source_model>Fastly\Cdn\Model\System\Config\Source\GeoIP\Action</source_model>
<depends>
<field id="enable_geoip">1</field>
</depends>
</field>
</group>
</group>
</group>
</section>
<section id="web" translate="label" type="text" sortOrder="20" showInDefault="1" showInWebsite="1" showInStore="1">
<label>Web</label>
<tab>general</tab>
<resource>Magento_Config::web</resource>
<group id="url" translate="label" type="text" sortOrder="3" showInDefault="1" showInWebsite="1" showInStore="1">
<label>Url Options</label>
<field id="use_store" translate="label comment" type="select" sortOrder="10" showInDefault="1" showInWebsite="0" showInStore="0" canRestore="1">
<label>Add Store Code to Urls</label>
<source_model>Magento\Config\Model\Config\Source\Yesno</source_model>
<backend_model>Magento\Config\Model\Config\Backend\Store</backend_model>
<comment>
<![CDATA[<strong style="color:red">Warning!</strong> When using Store Code in URLs, in some cases system may not work properly if URLs without Store Codes are specified in the third-party services (e.g. PayPal etc.)UPDATED.]]>
</comment>
</field>
<field id="test" translate="label comment" type="text" sortOrder="15" showInDefault="1" showInStore="1" showInWebsite="1">
<label>asdf</label>
<comment>If you see me, it means that module's system.xml was loaded.</comment>
</field>
<field id="redirect_to_base" translate="label comment" type="select" sortOrder="20" showInDefault="1" showInWebsite="1" showInStore="1" canRestore="1">
<label>Auto-redirect to Base URL</label>
<source_model>Magento\Config\Model\Config\Source\Web\Redirect</source_model>
<comment>I.e. redirect from http://example.com/store/ to http://www.example.com/store/</comment>
</field>
</group>
</section>
</system>
</config>
And I did no get field comments updated, but I can see my test "asdf" field:
That makes it even more interesting, doesn't it? :) My PHP version is: |
|
Hi @enl , let's check it again. Thanks for the update |
ghost
added
Progress: testing in progress
and removed
Progress: ready for testing
labels
|
✔️ QA Passed Before:
After:
|
ghost
added
Progress: extended testing
and removed
Progress: testing in progress
labels
magento-engcom-team
added
Component: Config
and removed
Component: Framework/Config
labels
|
Hi @enl, thank you for your contribution! |
magento-engcom-team
pushed a commit
that referenced
this pull request
Apr 23, 2019
…ig\Dom::merge` and fix failing ones #18336
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes several issues I found trying to update field comment of another module in Magento 2 Admin:
If I create another
system.xmlfile updating such a comment it will never be replaced because$this->_isTextNode($matchingNode)will always returnfalse: this<comment>node contains three childNodes, none of them is\DomElement(empty text node, cdata section, empty text node).During investigation of this, I created a bunch of new test cases for
Magento\Framework\Config\Dom::mergeand fixed failing ones.Manual testing scenarios (*)
Detailed steps described in THIS comment
Contribution checklist