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.

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

Fix Subfilter support #156

Open
wants to merge 9 commits into
base: 4.x
Choose a base branch
from
Open

Fix Subfilter support #156

wants to merge 9 commits into from

Conversation

harikt
Copy link
Member

@harikt harikt commented May 1, 2022

Related to #155 .

Support maximum of 2.

<?php
require __DIR__ . '/vendor/autoload.php';
$filter_factory = new \Aura\Filter\FilterFactory();
$filter = $filter_factory->newSubjectFilter();

$filter->validate('id')->is('int');
$filter->validate('url')->is('url');

$user_spec = $filter->subfilter('user'); // add a "SubSpec"
$user_filter = $user_spec->filter();  // Get the "SubSpec" SubjectFilter

$user_filter->validate('given-name')->isNotBlank();
$user_filter->validate('age')->is('int');
$user_filter->validate('gender')->is('strlen', 1);

$data = (object) [];

$result = $filter->apply($data);
$messages = $filter->getFailures()->getMessages();

var_dump($messages);

RESULT :

array(3) {
  ["id"]=>
  array(1) {
    [0]=>
    string(31) "id should have validated as int"
  }
  ["url"]=>
  array(1) {
    [0]=>
    string(32) "url should have validated as url"
  }
  ["user"]=>
  array(3) {
    ["given-name"]=>
    array(1) {
      [0]=>
      string(37) "given-name should not have been blank"
    }
    ["age"]=>
    array(1) {
      [0]=>
      string(32) "age should have validated as int"
    }
    ["gender"]=>
    array(1) {
      [0]=>
      string(41) "gender should have validated as strlen(1)"
    }
  }
}

@harikt harikt requested review from koriym and jakejohns May 1, 2022 17:38
@harikt
Copy link
Member Author

harikt commented May 1, 2022

The original PR was from @jakejohns #131 .

I spend quite sometime to make this happen. There are some glitches. But I am not sure how we can overcome that.

If anyone have better ways we can consider that also.

@harikt
Copy link
Member Author

harikt commented May 1, 2022

Anyone else interested in contributing can comment on this PR. I am happy to hear criticims. So no issues for raising the bar.

@@ -365,19 +365,23 @@ protected function applySpec(Spec $spec, object $subject): bool
*
*
*/
protected function failed(Spec $spec): Failure
protected function failed(Spec $spec)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is one place I dislike.

Copy link
Member

Choose a reason for hiding this comment

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

Is return void sufficient? The caller does not use the return value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Earlier it was returning the Failure. As it currently changed this now can return Failure or FailureCollection. ie why return is removed. I will look once again if this returned value has some value inside the caller.

Copy link
Member

Choose a reason for hiding this comment

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

This method is to add a failure. It seems no return value is needed.

@harikt harikt changed the title Maximum support upto 2 Fix Subfilter support May 9, 2022
tests/Spec/SubspecTest.php Outdated Show resolved Hide resolved
@koriym koriym requested a review from kenjis December 13, 2022 02:31
tests/Spec/SubspecTest.php Outdated Show resolved Hide resolved
Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

The implementation looks good to me.

tests/Spec/SubspecTest.php Outdated Show resolved Hide resolved
Co-authored-by: Akihito Koriyama <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants