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

Inconsistent results with nullable property #114

Closed
eigan opened this issue May 1, 2022 · 10 comments
Closed

Inconsistent results with nullable property #114

eigan opened this issue May 1, 2022 · 10 comments

Comments

@eigan
Copy link

eigan commented May 1, 2022

When the source is empty, is the mapper not able to set null to nullable property.

Reproduction

use CuyZ\Valinor\Mapper\MappingError;

require "vendor/autoload.php";

final class Foo
{
    public string|null $note;
}

final class Bar
{
    public string|null $note;

    public string $title;
}

try {
    $foo = (new \CuyZ\Valinor\MapperBuilder())
        ->mapper()
        ->map(Foo::class, []);
} catch (MappingError $e) {
    // Foo: Cannot cast value of type `array` to `string`.
    // Caused by ClassNodeBuilder::transformSource()
    // $source is [] and since the property $note does not exist in $source
    // will it return $source = [$name => $source];

    foreach($e->node()->children() as $child) {
        foreach($child->messages() as $message) {
            echo "Foo: " . $message . "\n";
        }
    }
}

// No issues:
$bar = (new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(Bar::class, ['title' => 'Foo']);

$bar->note === null; // true

Expected behavior

Foo::$note should be assigned null as value to be consistent with example Bar, but see my other issue here: #113.

@eigan
Copy link
Author

eigan commented May 1, 2022

Introducing a second nullable will make it possible to map.

final class Foo
{
    public string|null $note;
    public string|null $note2;
}


$foo = (new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(Foo::class, []);

$foo->note === null; // true

@eigan
Copy link
Author

eigan commented May 1, 2022

With an object instead of string are we able to map Foo, even though a single property in class.

final class Foo
{
    public Bar|null $bar;
}

class Bar {
    public string|null $title;
    public string|null $title2;
}

$foo = (new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(Foo::class, []);

$foo->bar instanceof Bar; // true

Introducing a new property yields different result. Foo::$bar is no longer a Bar.

final class Foo
{
    public $title; // New property

    public Bar|null $bar;
}

class Bar {
    public string|null $title;
    public string|null $title2;
}

$foo = (new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(Foo::class, []);

$foo->bar instanceof Bar; // false

@romm
Copy link
Member

romm commented May 2, 2022

Thanks for the excellent reproduction steps, I will definitely take a look at it.

Also, looks like it's bound to #34.

@romm
Copy link
Member

romm commented Jun 17, 2022

Hi @eigan, thanks again for the report, that helped a lot for #159. Could you try on your side to see if this leads to expected behaviors?

@eigan
Copy link
Author

eigan commented Jun 17, 2022

@romm Thanks! :)

Not sure if this fits here, but anyway, some testing:

<?php
require "vendor/autoload.php";

use CuyZ\Valinor\Mapper\MappingError;
use CuyZ\Valinor\Mapper\Tree\Node;

final class Foo
{
    public string $note = "ok";
    public Foo $foo;
}

try {
    (new \CuyZ\Valinor\MapperBuilder())
        ->mapper()
        ->map(Foo::class, []);
} catch (MappingError $e) {
    print_node($e->node());
}

function print_node(Node $node): void
{
    foreach ($node->messages() as $message) {
        echo $node->path() . ": " .$message . "\n";
    }

    foreach ($node->children() as $child) {
        print_node($child);
    }
}

Result

foo: Cannot be empty and must be filled with a value matching type `?`.

Expected

foo: Cannot be empty and must be filled with a value matching type `array{note?: string, foo: ?}|null`.

Nested type might be a bit tricky to render any further.. :)

@eigan
Copy link
Author

eigan commented Jun 17, 2022

<?php

require "vendor/autoload.php";

final class Foo
{
    public ?Foo $foo;
}

(new \CuyZ\Valinor\MapperBuilder())
    ->mapper()
    ->map(Foo::class, [
        'bar' => 'String'
    ]);

Result

PHP Fatal error:  Allowed memory size of 134217728 bytes exhausted (tried to allocate 262144 bytes) in src/Mapper/Tree/Builder/CasterProxyNodeBuilder.php on line 30

Changing type to Foo (not nullable) triggeres the same error, but from src/Mapper/Tree/Builder/ClassNodeBuilder.php on line 91

@eigan
Copy link
Author

eigan commented Jun 17, 2022

<?php

require "vendor/autoload.php";

use CuyZ\Valinor\Mapper\MappingError;
use CuyZ\Valinor\Mapper\Tree\Node;

final class Foo
{
    public ?string $bar = null;
}

try {
    (new \CuyZ\Valinor\MapperBuilder())
        ->mapper()
        ->map(Foo::class, []);
} catch (MappingError $e) {
    print_node($e->node());
}

function print_node(Node $node): void
{
    foreach ($node->messages() as $message) {
        echo $node->path() . ": " .$message . "\n";
    }

    foreach ($node->children() as $child) {
        print_node($child);
    }
}

Result

bar: Cannot be empty and must be filled with a value matching type `string|null`.

Expected

No error. $bar is nullable and has default value of null. There is no error when the default value is a string, so it is partially implemented.

@romm
Copy link
Member

romm commented Jun 20, 2022

Hi @eigan, really good catch with the nullable issue! Should be fixed with my last commit in #159, can you confirm?

Concerning the recursive issues, I can see the problem here, but I'll try to think about a solution in a separate PR. Thanks for the report though. 🙂

@eigan
Copy link
Author

eigan commented Jun 20, 2022

  • Issue with nullable and default null seems to be fixed 👍🏻
  • Memory issue still an issue, though seems to be an issue in master too.

Seems like you have solved all my points in this issue now 👏🏻


Recursive issue: Take a look at the following example. When sending a nested value, the error is reported as expected (type of Foo is exposed). So it seems to be sort-of implemented. But, I can agree that this is a non-blocking issue 🙂

require "vendor/autoload.php";

use CuyZ\Valinor\Mapper\MappingError;
use CuyZ\Valinor\Mapper\Tree\Node;

final class Foo
{
    public string $note = "hello";
    public ?Foo $foo;
}

try {
    (new \CuyZ\Valinor\MapperBuilder())
        ->mapper()
        ->map(Foo::class, [
            'foo' => [
                'foo' => 'ok'
            ]
        ]);
} catch (MappingError $e) {
    print_node($e->node());
}

function print_node(Node $node): void
{
    foreach ($node->messages() as $message) {
        echo $node->path() . ": " .$message . "\n";
    }

    foreach ($node->children() as $child) {
        print_node($child);
    }
}

Result

foo.foo: Value 'ok' does not match type `array{note?: string, foo: ?}`.

@romm
Copy link
Member

romm commented Jun 23, 2022

Fixed with #159.

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

No branches or pull requests

2 participants