Remove empty data after filtering in onRoute - option #102

rkeet · 2018-09-19T11:31:51Z

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

Are you creating a new feature?
- Why is the new feature needed? What purpose does it serve?

Issue #59 - When not using raw data (so filtered) and having requirements on the Entities (logically), the issue can exist that null values get attempted to be hydrated.

Filtering the data (by not using raw data), causes all keys for of all InputFilter's to be added to the data array which gets passed to the DoctrineObject hydrator in the current happy flow.

This PR gives an additional option: filter out the null values.

How will users use the new feature?

Same as use_raw_data and allows_only_fields_in_filter -> create config key with true value.

Base your feature on the develop branch, and submit against that branch.
Add only one feature per pull request; split multiple features over multiple pull requests
Add tests for the new feature.
Add documentation for the new feature.
Add a CHANGELOG.md entry for the new feature.

Acceptance criteria

Provide config flag to filter out null values
Filter out null values when flag set to true
Do not filter out null values from request (received) data, this is intentionally send and might be intended for optional value

This may not be the greatest solution, I'm open to suggestions/requests for improvement.

However, consider that this added bit does not change the current implementation, it just adds the possibility to filter out the null/empty values of a received data array.

It does not contain a bc-break as it must be enabled with config, the same way as 2 existing options.

…_data' config key.

…ter the data, added function to determine config setting, added overwriting of data with filtered data if key set and true in config.

…out the key - adds what it does and how to set it

coveralls · 2018-09-19T11:37:53Z

Coverage increased (+2.6%) to 81.513% when pulling c9fd599 on rkeet:feature/remove-empty-data into c36b369 on zfcampus:develop.

…ional checks for empty data on response of recursive calls to prevent possibility of returned empty array being left in final response.

weierophinney · 2018-10-31T13:49:34Z

src/ContentValidationListener.php

+ if (empty($value)) {
+ unset($data[$key]);
+ }
+ }


This can be simplified a ton:

foreach ($data as $key => $value) { if (! is_array($value) && ! empty($value)) { continue; } if (! is_array($value)) { unset($data[$key]); continue; } if (empty(array_filter($value, $removeNull))) { unset($data[$key]); continue; } $tmpValue = $this->removeEmptyData($value); if (empty(array_filter($tmpValue, $removeNull))) { unset($data[$key]); continue; } $data[$key] = $tmpValue; }

You don't need to unset $tmpValue as it gets re-assigned when the appropriate conditional occurs. By flattening the structure, we can better understand the various paths through the loop, which will make testing easier.

Ah thank you, will have a look at it again soon. Atm pushing deadlines, so might be a wee while.

weierophinney · 2018-10-31T13:51:31Z

test/ContentValidationListenerTest.php

 $this->assertEquals($params, $dataParams->getBodyParams());
 }

+ public function testFilterEmptyEntriesFromDataByOptionWithNestedData()


There are five different paths through the removeEmptyData() method, corresponding to five different conditions that can occur. You'll need five tests to fully exercise the method and verify it.

Will have another look. Was thinking today it wasn't enough, but as tests passed and I'm pushed for time, I was thinking of coming back to it.

rkeet · 2018-11-01T07:25:35Z

Woke up with the realization that this PR is slightly off at the moment. Will have to modify it some more (apart from @weierophinney's comments thus far).

E.g. an address: "Farmroad 3a", consists of 2 required (street, number) and 1 optional (addition) values.

But, the address was a typo, should've been "Farmroad 3". As addition is optional, it's nullable. So a patch request might contain just:

PATCH /address/2

{ "addition":null }

As such, I need to add in functionality to compare the data from which null's need to be removed against the data received in the request. This to ensure that data which should be hydrated as null does not get removed.

(btw, posting that here so I don't forget. I'm aiming to pick this up today or tomorrow, but it's busy at the office.)

… and clearer in use cases for data flow.

…t compatible with PHP < 7 and added TODO lines (2) for when module is bumpted to PHP > 7. Added comparisonData variable to onRoute function, later used for removeEmptyData, but it keeps unfiltered data instead of being overwritten at a later point.

rkeet · 2018-11-01T13:54:01Z

Must still update the tests

…ived data (compareTo) - current situation deleted them in next if statement - this is all in ContentValidationListener.

weierophinney

@rkeet I see you're still adding to this, so I did a quick review of what's here, and made a few more suggestions. In addition, I still need the tests I suggested before I can review to merge and release.

Thanks for all the great work so far!

weierophinney · 2019-01-29T21:43:27Z

src/ContentValidationListener.php

+ * @return array
+ */
+ // TODO strict types for PHP > 7 when module gets bumped
+ // protected function removeEmptyData(array $data, array $compareTo = []) : array


Put this in the docblock, above the @param annotations, as an @todo annotation, please.

weierophinney · 2019-01-29T21:44:30Z

src/ContentValidationListener.php

+ protected function removeEmptyData($data, $compareTo = [])
+ {
+ // TODO when bumped to PHP > 7 and strict types above enforced this check may be removed
+ if (! is_array($data) || ! is_array($compareTo)) {


PHP 5.6 allows us to use the array typehint for arguments.; we do not need to wait for PHP 7 here.

weierophinney · 2019-01-29T21:48:04Z

src/ContentValidationListener.php

+ $removeNull = function ($value, $key = null) use ($compareTo) {
+ // If comparison array is empty, do a straight comparison
+ if (empty($compareTo)) {
+ return ! is_null($value);


We tend to use the construct null !== $value.

weierophinney · 2019-01-29T21:48:24Z

src/ContentValidationListener.php

+ return true;
+ }
+
+ return ! is_null($value);


Same comment here: null !== $value

weierophinney · 2019-01-29T21:51:14Z

src/ContentValidationListener.php

+ $tmpValue = $this->removeEmptyData($value, $compareTo[$key]);
+ } else {
+ $tmpValue = $this->removeEmptyData($value);
+ }


This could become:

$tmpValue = array_key_exists($key, $compareTo) && is_array($compareTo[$key]) ? $this->removeEmptyData($value, $compareTo[$key]) : $this->removeEmptyData($value);

Ternary assignments are nice for this kind of either/or thing, and make reading the code easier.

…ary. Replaced '! is_null()' with 'null !== '.

rkeet · 2019-02-03T18:44:38Z

TODO

Unit testing through added functionality (current tests pass, but do not cover all use cases)

…ContentValidationListener

…tionality for creatoin of listener and event into own functions, re-used for all 5 test functions

rkeet · 2019-02-03T19:39:08Z

I think I've got it all covered now. Please do check.

rkeet · 2019-02-25T14:23:32Z

Small ping @weierophinney

- Removed unnecessary import statement. - Simplified conditional in `shouldRemoveEmptyData()` to remove redundant `isset` check. - Removed `@todo` annotations that detail future signature changes; these will happen regardless. - Add parameter types to annotations where necessary. - Remove unnecessary parens. - Remove unnecessary whitespace.

- Extracts the `booleanProvider` for use with `testFilterEmptyEntriesFromDataByOptionWhenValueBooleanNotInComparison()` test case. - Moves annotations to the end of method docblocks. - If the only argument is an array, moves opening brace to same line as method invocation. - Ensure arrays have trailing commas.

weierophinney

I've pushed a few minor consistency changes, mostly around formatting; otherwise, this is ready to go!

Thanks, @rkeet !

diego-sorribas · 2019-03-01T12:19:02Z

Hello, I have found some problems in my api with this new PR. I have some forms that send zero value and this new PR unset me the body param. Its that ok? I need to set remove_empty_data in my config for every controller? Im using Apigility.

michalbundyra · 2019-03-02T18:54:34Z

src/ContentValidationListener.php

+ protected function shouldRemoveEmptyData($controllerService)
+ {
+ if (! isset($this->config[$controllerService]['remove_empty_data'])
+ || $this->config[$controllerService]['remove_empty_data'] === true


This condition is wrong, please see #104 and #105.

michalbundyra · 2019-03-16T14:34:28Z

@diego-sorribas It is already fixed in version 1.7.1 - see:
https://github.com/zfcampus/zf-content-validation/releases/tag/1.7.1

rkeet-salesupply added 3 commits September 19, 2018 13:22

Created test for removal of empty key/value pairs using 'remove_empty…

4d05231

…_data' config key.

Created implementation for 'remove_empty_data', added function to fil…

024b5e0

…ter the data, added function to determine config setting, added overwriting of data with filtered data if key set and true in config.

updated CHANGELOG.md, README.md and module config with information ab…

efdd02a

…out the key - adds what it does and how to set it

Removal of empty lines for code style check

d55feab

rkeet mentioned this pull request Sep 24, 2018

Feature/remove empty data rkeet/zf-content-validation#1

Merged

Updated removeEmptyData in ContentValidationListener to perform addit…

511b3c7

…ional checks for empty data on response of recursive calls to prevent possibility of returned empty array being left in final response.

weierophinney suggested changes Oct 31, 2018

View reviewed changes

rkeet-salesupply added 3 commits November 1, 2018 14:48

Simplified removeEmptyData function based on feedback. Easier to read…

b828256

… and clearer in use cases for data flow.

Updated README.md description of 'remove_empty_data' flag

686a45d

Removed few spaces for code style checks

5084634

rkeet force-pushed the feature/remove-empty-data branch from a57d74f to 5084634 Compare November 1, 2018 14:21

Fix to ensure that boolean values are allowed and get checked vs rece…

2d9af12

…ived data (compareTo) - current situation deleted them in next if statement - this is all in ContentValidationListener.

weierophinney suggested changes Jan 29, 2019

View reviewed changes

Code style comments on PR - Placed todo's in annotation. Applied tern…

28dc1bd

…ary. Replaced '! is_null()' with 'null !== '.

rkeet added 2 commits February 3, 2019 20:36

Moved inline comment about anon function into function annotation in …

50a4143

…ContentValidationListener

Created tests per use-case for removeEmptyData - split recurring func…

818f22f

…tionality for creatoin of listener and event into own functions, re-used for all 5 test functions

weierophinney added 3 commits February 26, 2019 12:32

docs: expands description of zfcampus#102 in CHANGELOG

c9fd599

weierophinney approved these changes Feb 26, 2019

View reviewed changes

weierophinney merged commit c9fd599 into zfcampus:develop Feb 26, 2019

michalbundyra reviewed Mar 2, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove empty data after filtering in onRoute - option #102

Remove empty data after filtering in onRoute - option #102

Uh oh!

rkeet commented Sep 19, 2018 •

edited

Loading

coveralls commented Sep 19, 2018 •

edited

Loading

weierophinney Oct 31, 2018

rkeet Oct 31, 2018

rkeet Nov 1, 2018

weierophinney Oct 31, 2018

rkeet Oct 31, 2018

rkeet commented Nov 1, 2018 •

edited

Loading

rkeet commented Nov 1, 2018

weierophinney left a comment

weierophinney Jan 29, 2019

rkeet Feb 3, 2019

weierophinney Jan 29, 2019

rkeet Feb 3, 2019

weierophinney Jan 29, 2019

rkeet Feb 3, 2019

weierophinney Jan 29, 2019

rkeet Feb 3, 2019

weierophinney Jan 29, 2019

rkeet Feb 3, 2019

rkeet commented Feb 3, 2019

rkeet commented Feb 3, 2019

rkeet commented Feb 25, 2019

weierophinney left a comment

diego-sorribas commented Mar 1, 2019

michalbundyra Mar 2, 2019

michalbundyra commented Mar 16, 2019

Labels

6 participants

Remove empty data after filtering in onRoute - option #102

Remove empty data after filtering in onRoute - option #102

Uh oh!

Conversation

rkeet commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

coveralls commented Sep 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

rkeet commented Nov 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

rkeet commented Nov 1, 2018

weierophinney 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

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

rkeet commented Feb 3, 2019

rkeet commented Feb 3, 2019

rkeet commented Feb 25, 2019

weierophinney left a comment

Choose a reason for hiding this comment

diego-sorribas commented Mar 1, 2019

Choose a reason for hiding this comment

michalbundyra commented Mar 16, 2019

Labels

6 participants

rkeet commented Sep 19, 2018 •

edited

Loading

coveralls commented Sep 19, 2018 •

edited

Loading

rkeet commented Nov 1, 2018 •

edited

Loading