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.

Sign up for GitHub

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

Jump to bottom

Return empty string for empty xml tag in modRestService #14305

Merged
merged 1 commit into from
Mar 6, 2019
Merged

Return empty string for empty xml tag in modRestService #14305

merged 1 commit into from
Mar 6, 2019

Conversation

ilyautkin
Copy link
Contributor

What does it do?

Check output for empty array. If it is empty, return empty string.

Why is it needed?

When modRestService parse XML to array empty XML-tags returning as empty array.

Related issue(s)/PR(s)

#13132

@ilyautkin
Copy link
Contributor Author

How to test this PR

Create file test.php in the MODX root:

<?php
define('MODX_API_MODE', true);
require 'index.php';

$modx->getService('rest', 'rest.modRestService');
class customRequest extends modRestServiceRequest {
    public function _xml2array(SimpleXMLElement $xml,$attributesKey=null,$childrenKey=null,$valueKey=null) {
        return parent::_xml2array($xml);
    }
}
$request = new customRequest($modx->rest);

$data = <<<XML
<?xml version='1.0'?> 
<document>
  <title>Title</title>
  <empty></empty>
  <name>John</name>
  <children>
    <child><title>child</title></child>
    <child><title></title></child>
  </children>
  <body>Text</body>
</document>
XML;
$xml = simplexml_load_string($data);
$array = $request->_xml2array($xml);

echo '<pre>';
var_dump($array);
echo '</pre>';

Check empty fields before and after code changes.

@Jako Jako added this to the v3.0.0-alpha milestone Jan 23, 2019
@Jako Jako added area-core proposal Proposal about improvement aka RFC. Need to be discussed before start implementation. pr/review-needed Pull request requires review and testing. labels Jan 23, 2019
Ruslan-Aleev
Ruslan-Aleev approved these changes Jan 25, 2019
View reviewed changes
Copy link
Collaborator

@Ruslan-Aleev Ruslan-Aleev left a comment

Choose a reason for hiding this comment

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

Tested, works as planned.
Before:

array(5) {
  ["title"]=>
  string(5) "Title"
  ["empty"]=>
  array(0) {
  }
  ["name"]=>
  string(4) "John"
  ["children"]=>
  array(1) {
    ["child"]=>
    array(2) {
      [0]=>
      array(1) {
        ["title"]=>
        string(5) "child"
      }
      [1]=>
      array(1) {
        ["title"]=>
        array(0) {
        }
      }
    }
  }
  ["body"]=>
  string(4) "Text"
}

After:

array(5) {
  ["title"]=>
  string(5) "Title"
  ["empty"]=>
  string(0) ""
  ["name"]=>
  string(4) "John"
  ["children"]=>
  array(1) {
    ["child"]=>
    array(2) {
      [0]=>
      array(1) {
        ["title"]=>
        string(5) "child"
      }
      [1]=>
      array(1) {
        ["title"]=>
        string(0) ""
      }
    }
  }
  ["body"]=>
  string(4) "Text"
}

@Mark-H
Copy link
Collaborator

Mark-H commented Jan 29, 2019

Hmm, how can you tell from the raw XML if an entry is supposed to be a string or an array?

With the example for testing (thanks for that!) it's obvious that you want empty as a string. But what if <children> was empty, wouldn't you want an empty array?

@ilyautkin
Copy link
Contributor Author

@Mark-H, in comments was decided that this is a bug. #13132:

With the current code it is not possible to update a record value with an empty value using the default put method

@Mark-H
Copy link
Collaborator

Mark-H commented Feb 4, 2019

Ahh this is for handling incoming requests, I thought it was for processing responses. My bad.

@alroniks alroniks added pr/ready-for-merging Pull request reviewed and tested and ready for merging. and removed pr/review-needed Pull request requires review and testing. labels Mar 6, 2019
@alroniks alroniks merged commit 21866d7 into modxcms:3.x Mar 6, 2019
alroniks pushed a commit that referenced this pull request Mar 6, 2019
Return empty string for empty xml tag in modRestService [#14305]
2a39423 
* upstream/pr/14305:
  Return empty string for empty xml tag in modRestService
@Mark-H Mark-H mentioned this pull request Nov 27, 2019
Open
53 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alroniks alroniks alroniks approved these changes

@Mark-H Mark-H Mark-H approved these changes

@Ruslan-Aleev Ruslan-Aleev Ruslan-Aleev approved these changes

@opengeek opengeek Awaiting requested review from opengeek opengeek is a code owner

Assignees
No one assigned
Labels
area-core pr/ready-for-merging Pull request reviewed and tested and ready for merging. proposal Proposal about improvement aka RFC. Need to be discussed before start implementation.
Projects
None yet
Milestone
v3.0.0-alpha1
Development

Successfully merging this pull request may close these issues.
5 participants
@ilyautkin @Mark-H @alroniks @Ruslan-Aleev @Jako