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

arrayOf, listOf defaults are always in array #13

Closed
MartkCz opened this issue Oct 19, 2019 · 7 comments
Closed

arrayOf, listOf defaults are always in array #13

MartkCz opened this issue Oct 19, 2019 · 7 comments

Comments

@MartkCz
Copy link

MartkCz commented Oct 19, 2019

Version: 1.0.0

$schema = Expect::listOf('string')->default([
	'foo',
	'bar',
]);

$processor = new Processor();

$array = $processor->process($schema, [
	'foo',
	'bar'
]);

var_dump($array);

dumps:

array(4) {
  [0]=>
  string(3) "foo"
  [1]=>
  string(3) "bar"
  [2]=>
  string(3) "foo"
  [3]=>
  string(3) "bar"
}
$schema = Expect::arrayOf('string')->default([
	'foo',
	'bar',
]);

$processor = new Processor();

$array = $processor->process($schema, [
	'foo',
	'bar'
]);

var_dump($array);

dumps:

array(4) {
  [0]=>
  string(3) "foo"
  [1]=>
  string(3) "bar"
  [2]=>
  string(3) "foo"
  [3]=>
  string(3) "bar"
}

Is it the right behavior?

@dg
Copy link
Member

dg commented Oct 22, 2019

It is an intention, although I realize that it is not completely understandable. It is used for example for the configuration of http > headers.

@mabar
Copy link
Contributor

mabar commented Oct 22, 2019

@MartkCz Lists are not supported by PHP, so result is same. Difference is in validation - lists does not support keys https://github.com/nette/schema/blob/master/tests/Schema/Expect.list.phpt#L14-L22

Edit: I see, default values are always included in result array, sorry

@MartkCz
Copy link
Author

MartkCz commented Oct 24, 2019

@mabar It's issue only about default values for array/list :)

@dg I undestand

@MartkCz MartkCz closed this as completed Oct 24, 2019
@dg
Copy link
Member

dg commented Oct 29, 2019

I will leave it open as a reminder that this is not ideal.

@dg dg reopened this Oct 29, 2019
@AdryanSignor
Copy link

Hi, @MartkCz, @dg, a temporary "solution" to applying the default is to use the before method:

  • Merge values
->before(function($value) {
    $default = ['foo', 'bar'];

    if ($value === null) {
        $value = $default;
    } elseif (is_array($value)) {
        $value = array_merge($default, array_diff($value, $default));
    }

    return $value;
});
  • Default empty
->before(function($value) {
    return $value === null || $value === [] ? ['foo', 'bar'] : $value;
})

@colinodell
Copy link

This workaround doesn't seem to work when the option is nested within a Structure and no value is provided by the user:

test('listOf workaround from #13', function () {
    // Test current behavior of always merging
    $schema = Expect::listOf('int')->default([1, 2, 3]);

    Assert::same([1, 2, 3], (new Processor)->process($schema, null));
    Assert::same([1, 2, 3], (new Processor)->process($schema, []));
    Assert::same([1, 2, 3, 42], (new Processor)->process($schema, [42])); // Not the behavior I want; I don't want the defaults here

    // Test workaround from #13
    $schema = Expect::listOf('int')->before(function($value) {
        return $value === null || $value === [] ? [1, 2, 3] : $value;
    });

    Assert::same([1, 2, 3], (new Processor)->process($schema, null));
    Assert::same([1, 2, 3], (new Processor)->process($schema, []));
    Assert::same([42], (new Processor)->process($schema, [42])); // Workaround seems to function fine in this particular case

    // Test current behavior of always merging within a nested structure
    $schema = Expect::structure([
        'test' => Expect::listOf('int')->default([1, 2, 3])
    ])->castTo('array');

    Assert::same(['test' => [1, 2, 3]], (new Processor)->process($schema, null));
    Assert::same(['test' => [1, 2, 3]], (new Processor)->process($schema, []));
    Assert::same(['test' => [1, 2, 3]], (new Processor)->process($schema, ['test' => null]));
    Assert::same(['test' => [1, 2, 3]], (new Processor)->process($schema, ['test' => []]));
    Assert::same(['test' => [1, 2, 3, 42]], (new Processor)->process($schema, ['test' => [42]])); // Not the behavior I want; I don't want the defaults here

    // Test workaround from #13 within a nested structure
    $schema = Expect::structure([
        'test' => Expect::listOf('int')->before(function($value) {
            return $value === null || $value === [] ? [1, 2, 3] : $value;
        })
    ])->castTo('array');

    Assert::same(['test' => []], (new Processor)->process($schema, null)); // Not the behavior I want (defaults should ideally be applied if option missing)
    Assert::same(['test' => []], (new Processor)->process($schema, [])); // Not the behavior I want (defaults should ideally be applied if option missing)
    Assert::same(['test' => [1, 2, 3]], (new Processor)->process($schema, ['test' => null]));
    Assert::same(['test' => [1, 2, 3]], (new Processor)->process($schema, ['test' => []]));
    Assert::same(['test' => [42]], (new Processor)->process($schema, ['test' => [42]]));
});

@AdryanSignor
Copy link

AdryanSignor commented Oct 17, 2020

Hi @colinodell, I agree. Sometimes I had to use array_replace_recursive on user's data, for example:

$schema = Expect::structure([
    'foo' => Expect::listOf('string')->before(function ($v) {
        return $v === null || $v === [] ? ['bar'] : $v;
    })
]);

$defaultStructure = [
    'foo' => null
];

$userData = [];

// Excepts
// class stdClass#11 (1) {
//  public $foo =>
// array(1) {
//    [0] =>
//    string(3) "bar"
//  }
// }

$processor->process($schema, $userData); // Doesn't work
$processor->process($schema, array_replace_recursive($defaultStructure, $userData)); // Works

I tested your solution (#28) on my projects, replacing before method by default([...])->preventMergingDefaults() and removing $defaultStructure from user's data. Much better 😆.

🚀🚀🚀

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 a pull request may close this issue.

5 participants