Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Allow empty array in data #37

Merged
merged 2 commits into from
Apr 4, 2018
Merged

Allow empty array in data #37

merged 2 commits into from
Apr 4, 2018

Conversation

MaSpeng
Copy link
Contributor

@MaSpeng MaSpeng commented Mar 28, 2018

Provide a narrative description of what you are trying to accomplish:

Checklist

  • Are you fixing a bug?
    • Detail how the bug is invoked currently.
    • Detail the original, incorrect behavior.
    • Detail the new, expected behavior.
    • Base your feature on the master branch, and submit against that branch.
    • Add a regression test that demonstrates the bug, and proves the fix.
    • Add a CHANGELOG.md entry for the fix.

Detail how the bug is invoked currently

The bug could currently invoked as follows:
$resource = new HalResource(
    [
        'empty_list' => [],
    ]
);

Detail the original, incorrect behavior

The original, incorrect behavior is as follows:
$data = [
    'integer_value' => 1,
    'boolean_value' => true,
    'string_value' => 'string',
    'null_value' => null,
    'associative_array' => [
        'key' => 'value',
        'empty_array' => [],
    ],
    'empty_array' => [],
];
$links = [];
$embedded = [];

$resource = new \Zend\Expressive\Hal\HalResource($data, $links, $embedded);

var_export($resource->toArray()); // -> will produce the following output:

/*
[
    'integer_value' => 1,
    'boolean_value' => true,
    'string_value' => 'string',
    'null_value' => null,
    'appositive_array' => [
        'key' => 'value',
        'empty_array' => [
        ],
    ],
    '_embedded' => [
        'empty_array' => [
        ],
    ],
];
*/

The empty_array within data $data array is inserted into the _embedded key instead of remaining with the other values.

Detail the new, expected behavior

The new, expected behavior should be as follows:
$data = [
    'integer_value' => 1,
    'boolean_value' => true,
    'string_value' => 'string',
    'null_value' => null,
    'associative_array' => [
        'key' => 'value',
        'empty_array' => [],
    ],
    'empty_array' => [],
];
$links = [];
$embedded = [];

$resource = new \Zend\Expressive\Hal\HalResource($data, $links, $embedded);

var_export($resource->toArray()); // -> will produce the following output:

/*
[
    'integer_value' => 1,
    'boolean_value' => true,
    'string_value' => 'string',
    'null_value' => null,
    'associative_array' => [
        'key' => 'value',
        'empty_list' => [],
    ],
    'empty_list' => [],
];
*/

The empty_array within data $data array remains with the other values.

Examples how to create an item and an collection resource

Example how an item resource will be created
$data = [
    'identifier' => 432143,
    'first_name' => 'John',
    'last_name' => 'Doe',
    'roles' => [],
];
$links = [
    new \Zend\Expressive\Hal\Link(
        'self',
        '/users/432143'
    ),
];
$embedded = [];

$resource = new \Zend\Expressive\Hal\HalResource($data, $links, $embedded);

var_export($resource->toArray()); // -> will produce the following output:

/* 
[
    'identifier' => 432143,
    'first_name' => 'John',
    'last_name' => 'Doe',
    'roles' => [],
    '_links' => [
        '_self' => [
            'href' => '/users/432143',
        ],
    ],
]
/*
Example how an collection resource will be created, for example as an result of an search. This is also the current behavior when using the `ResourceGenerator`.
$data = [
    '_total_items' => 0,
    '_page' => 1,
    '_page_count' => 0,
];
$links = [
    new \Zend\Expressive\Hal\Link(
        'self',
        '/users?page=1&limit=10&search=Jane'
    ),
];
$embedded = [
    'users' => [],
];

$resource = new \Zend\Expressive\Hal\HalResource($data, $links, $embedded);

var_export($resource->toArray()); // -> will produce the following output:

/*    
[
    '_total_items' => 0,
    '_page' => 1,
    '_page_count' => 0,
    '_links' => [
        'self' => [
            'href' => '/users?page=1&limit=10&search=Jane',
        ],
    ],
    '_embedded' => [
        'users' => [],
    ],
]
*/

Example how to explicit embed an empty list

Describing how to embed an empty array inside of an created **resource**:
$resource = new \Zend\Expressive\Hal\HalResource();

$withEmbedded = $resource->embed('users', []);

var_export($withEmbedded->toArray()); // -> will produce the following output:

/*
[
    '_embedded' => [
        'users' => [],
    ],
]
*/
This PR should solve issue: [#34](https://github.com//issues/34)

MaSpeng added 2 commits March 28, 2018 22:04
- added check for empty array in data to avoid unnecessary embedded data with empty lists
- added changelog entry regarding fix
- fixed code style issues
@weierophinney
Copy link
Member

Per issue #34, what happens if, say, a resource is empty, and you do want it represented? For example, if you the request was a search, and you would expect an array of values, even if that array is empty?

Based on the discussion on that issue and the tests here, it seems like you are allowing either use case, but it's unclear to me currently if the syntax for doing so differs, and, if so, what I would need to do to achieve one or the other behavior.

Can you add some documentation to clarify how to achieve either result, please?

@MaSpeng
Copy link
Contributor Author

MaSpeng commented Mar 29, 2018

Thanks, for your advice I have updated the pr description to better reflect what I'am trying to accomplish.

@weierophinney weierophinney merged commit f7a4179 into zendframework:master Apr 4, 2018
weierophinney added a commit that referenced this pull request Apr 4, 2018
weierophinney added a commit that referenced this pull request Apr 4, 2018
- `s/34/37/` in description.
- Better description describing change in behavior, and how to get
  original behavior back.
weierophinney added a commit that referenced this pull request Apr 4, 2018
weierophinney added a commit that referenced this pull request Apr 4, 2018
Forward port #37

Conflicts:
	CHANGELOG.md
@weierophinney
Copy link
Member

Thanks, @MaSpeng!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants