Add support for multiple space-separated CDATA sections in Magento\Framework\Xml\Parser

[fnogatz]

Description (*)

Our XML files sometimes contain multiple CDATA sections per node. The XML parser class provided by Magento in /lib/internal/Magento/Framework/Xml/Parser.php already supports consecutive CDATA sections like <![CDATA[One]]><![CDATA[Two]]><![CDATA[Three]]>, which is correctly parsed as OneTwoThree. However this returns wrong results for empty text nodes in-between, like for <![CDATA[One]]> <![CDATA[Two]]> <![CDATA[Three]]>. This pull request adds the support and a related unit test.

Related Pull Requests

The switch statement has been added in #681.

Fixed Issues (if relevant)

No separate issue created.

Manual testing scenarios (*)

Unit tests added as part of this pull request.

You can use the current 2.3.4 version for testing. The file in question /lib/internal/Magento/Framework/Xml/Parser.php is unchanged since August 2017.

Questions or comments

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 are green)

Resolved issues:

1. resolves [Issue] Add support for multiple space-separated CDATA sections in Magento\Framework\Xml\Parser #29613: Add support for multiple space-separated CDATA sections in Magento\Framework\Xml\Parser

[m2-assistant]

Hi @fnogatz. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[sidolov]

@magento create issue

[ihor-sviziev]

@magento run all tests

[ihor-sviziev]

@magento run Unit tests, Static Tests

[ihor-sviziev]

@magento run Unit Tests

[ihor-sviziev]

Hi @fnogatz,
Thank you for updating your PR, now it looks really good.
The only thing left - could you please also fix failing static tests? I know that this failure not caused by your changes, but still need to have not failing static tests for changed files.

Thank you!

[ihor-sviziev]

✔ Approved.

Failing tests looks not related to changes form this PR.

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8171 has been created to process this Pull Request

[magento-engcom-team]

@fnogatz thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[ihor-sviziev]

@magento run all tests

[engcom-Delta]

Hi @fnogatz . During testing PR changes I tried to add multiple CDATA sections into label and there are results:
Case 1: add multiple CDATA into comment node

Result:
❌ No comment is shown under field

Case 2: add multiple CDATA into label node

Result:
❌ Exception error "Notice: Array to string conversion in /var/www/html/magento2dev/lib/internal/Magento/Framework/Phrase.php on line 72 in log

Could you clarify in what cases multiple CDATA is used?

[fnogatz]

Hi @engcom-Delta. Thank you for having a look into it! We use the XML parser class provided by Magento in /lib/internal/Magento/Framework/Xml/Parser.php to process XML files not related to Magento, i.e. in a preprocessing step on our proprietary data-import. So the issue did not arise from a scenario where we tried to add multiple CDATA sections on Magento's files like config.xml, system.xml, widget.xml, or the like.

However, I tried to reproduce the issue on the Magento backend as you did by modifying Magento's XML files. To be honest, after two hours I have no clue why it behaves differently. Here's my Case 3:

• Modify the description of cache_lifetime in magento/module-catalog-widget/etc/widget.xml to contain mixed CDATA and text, e.g., <![CDATA[ Some data]]> here<![CDATA[ <strong>html</strong> tags]]> are <![CDATA[<i>allowed</i> ]]>:
  
• Even without the modifications proposed in this PR, this gets correctly (!) displayed in the Magento backend:
  
• However, if I load this modified widget.xml programmatically and run xmlToArray(), it produces only the value string(16) "<i>allowed</i> ":

  $parser = new Parser();
  $parser->load('magento/module-catalog-widget/etc/widget.xml');
  $content = $parser->xmlToArray();    
  var_dump($content);
  /*
      ...
      ["_value"]=>
      array(2) {
        ["label"]=>
        array(2) {
          ["_value"]=>
          string(24) "Cache Lifetime (Seconds)"
          ["_attribute"]=>
          array(1) {
            ["translate"]=>
            string(4) "true"
          }
        }
        ["description"]=>
        array(2) {
          ["_value"]=>
          string(16) "<i>allowed</i>  "
          ["_attribute"]=>
          array(1) {
            ["translate"]=>
            string(4) "true"
          }
        }
      }
      ...
  */

  The expected string(64) " Some data here <strong>html</strong> tags are <i>allowed</i> " is returned only after applying this PR.

I have no idea, how the full description can be shown in the Magento backend without this PR, whereas manually loading the XML file results in an incomplete string at the same time -- which makes it hard for me to debug :/

[ihor-sviziev]

@engcom-Charlie @engcom-Delta @engcom-Bravo could you help with that?

[engcom-Charlie]

@fnogatz you can see merged widget.xml files as array for example like this

$widgetReader = $om->create(\Magento\Widget\Model\Config\Reader::class);
$res = $widgetReader->read();
var_dump($res);

[engcom-Charlie]

Hi, @fnogatz could you please provide steps to reproduce the issue?
Thank you.

[ihor-sviziev]

Hi @fnogatz,
Could you check if the issue still reproducing? Could you add steps to reproduce?

[sidolov]

@fnogatz I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

[m2-assistant]

Hi @fnogatz, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.