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

Support class constants properties #119

Closed
eigan opened this issue May 4, 2022 · 10 comments · Fixed by #229
Closed

Support class constants properties #119

eigan opened this issue May 4, 2022 · 10 comments · Fixed by #229

Comments

@eigan
Copy link

eigan commented May 4, 2022

As defined at phpstan here: https://phpstan.org/writing-php-code/phpdoc-types#literals-and-constants

class DtoValues {
    public const FOO = 1;
    public const FOO_BAR = 2;
    public const BAR = 'FooBar!';
}

class FooBar
{
    /**
     * @var DtoValues::FOO*
     */
    public $id;

    /**
     * @var DtoValues::BAR|null
     */
    public $title;
}

Actual use-case

Optional properties in payload.

class DtoValues {
    public const None = 'SomeNoneValueString';
}

class WithOptionalDto
{
    /**
     * @var DtoValues::None|null|int
     */
    public $priority = DtoValues::None;
}

// Payload does not need to contain `priority`, it has a default value (None).
$object = buildDto(WithOptionalDto::class, $source);

if ($object->priority !== DtoValues::None) {
    // Payload actually contains some value, lets use it.
}

Today, the error message is:

CuyZ\Valinor\Definition\Exception\InvalidPropertyDefaultValue: Default value of property WithOptionalDto::$priority is not accepted by DtoValues
image
(Screenshot taken in ReflectionPropertyDefinitionBuilder)

Notice it simply ignores the rest of the types, and is not even an UnionType as expected.

In PHP 8.1 is this a lot easier because we could use a global const which value is an object (const None = new None) and use None as a valid type. Though we are still on 7.4 and must rely on some weird strings - if we decide that this is the best approuch for optional properties.

@eigan
Copy link
Author

eigan commented May 6, 2022

I tried to implement a simple ConstantType, but seems to need a lot of changes, more than I am comfortable 'just doing'.

Example

const FOO = 'bar';
const FOO_BAR = 'bar2';

class  Bar {
    /**
     * @var FOO|FOO_BAR
     */
    public $type = FOO;
}

To implement ConstantType::matches(Type $other): bool do we need to know the value of the Type. ReflectionTypeResolver will resolve the default value of $type here as NativeStringType. We cannot match with that. Not sure how to solve this, maybe NativeStringType could accept a defaultValue?

@romm
Copy link
Member

romm commented May 6, 2022

Hi @eigan, I'd be glad to help you when you're ready to submit a PR.

Concerning your issue, I think you should take a look at StringValueType instead of NativeStringType, that should help you. Same if the constant value is an integer: you can use IntegerValueType.

Don't hesitate if you need more help!

@eigan
Copy link
Author

eigan commented May 6, 2022

@romm I did try StringValueType, but then the type is FixedType and casting is no longer supported.

Consider the case:

class Bar {
    /**
      * @var string
      */
    public $type = "fiz";
}

Type of Bar::$type is now StringValueType an cannot be cast to say "bar" if you send that in $source. Lots of tests failed because of this. Maybe I need to change to StringValueType somewhere else than ReflectionTypeResolver..

See wip here https://github.com/eigan/Valinor/pull/1/files

@romm
Copy link
Member

romm commented May 6, 2022

I see, this might not be as easy as it first seemed to be.

At first I thought the ConstantType should act as a decorator somehow, and should handle both scalar (string, int, float and bool) and array values.

But now I wonder if there should just not be any ConstantType at all, and the lexer should directly map to existing types.

class Foo
{
    public const SOME_CONST = 'foo';

    /** @var Foo::SOME_CONST */
    public string $foo;
}

—> the resolved type would be StringValueType('foo')

class Foo
{
    public const SOME_CONST = 42;

    /** @var Foo::SOME_CONST */
    public int $foo;
}

—> the resolved type would be IntegerValueType(42)

class Foo
{
    public const SOME_CONST = 1337.42;

    /** @var Foo::SOME_CONST */
    public float $foo;
}

—> FloatValueType does not exist (yet?) 😬

class Foo
{
    public const SOME_CONST = true;

    /** @var Foo::SOME_CONST */
    public bool $foo;
}

—> Well this one is coming in #117 😄

class Foo
{
    public const SOME_CONST = [
        'foo' => 'foo',
        'bar' => 'bar',
    ];

    /** @var Foo::SOME_CONST */
    public array $foo;
}

—> the resolved type would be ShapedArrayType with two elements describing the array (it should also work with nested arrays).

class Foo
{
    public const SOME_CONST_A = 'foo';
    public const SOME_CONST_B = 42;

    /** @var Foo::SOME_CONST_* */
    public string|int $foo;
}

—> the resolved type would be UnionType(StringValueType('foo'), IntegerValueType(42))

Probably a lot of work ahead 😬

@eigan
Copy link
Author

eigan commented May 6, 2022

class Bar {}

const SOME_CONST = new Bar;

class Foo
{

    /** @var SOME_CONST|string */
    public $foo;
}

What would the resolved type be in this case?

@romm
Copy link
Member

romm commented May 6, 2022

Damn I just learned something!

This looks like a twisted case, but this would be possible so that should be handled as well. I'll have to think a bit about it.

If you're ok with it, I will help you out with the lexing part which could be a bit hard (unless you want to try to do it by yourself), then you should be able to finish it. I'll keep you posted. 🙂

@eigan
Copy link
Author

eigan commented May 6, 2022

I am not a "lexer" guy (no experience here), but want to help. Writing tests or whatever :)

My initial thought after your last comment was to try return the correct token after resolving the value of the constant. Doesn't sound that difficult, but might say more about my experience than anything... :)

Please let me know when you think about starting with this, it's something we need so I am especially eager to help, or start by my self if you have other priorities :)

@romm
Copy link
Member

romm commented May 9, 2022

Hi @eigan, I just started with #126.

I'll keep working on the constants handling feature and keep you updated. The best you will be able to do is probably review the PR, maybe add tests if you see that some are missing, and finally test the feature on your side and see if everything is ok. 🙂

@romm
Copy link
Member

romm commented Oct 6, 2022

Hi @eigan, after all this time the feature finally made it with the 0.15.0 release!

Thank you for your patience!

@eigan
Copy link
Author

eigan commented Oct 6, 2022

@romm Thanks! 😃

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.

2 participants