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

Entity property with unknown type is ignored #521

Open
mabar opened this issue Jun 2, 2021 · 9 comments
Open

Entity property with unknown type is ignored #521

mabar opened this issue Jun 2, 2021 · 9 comments

Comments

@mabar
Copy link
Contributor

mabar commented Jun 2, 2021

Describe the bug

Properties using generic array syntax and defining key type are ignored.

To Reproduce

  1. Create an entity (of type Nextras\Orm\Entity\Entity)
  2. Add property @property array<string, string> $example
  3. Assign value to $example property, EntityMetadata should throw exception

Expected behavior

@property array<string, string> $example is a valid property, same as (currently valid) @property array<string> $example or at least throw an exception describing syntax is not supported.

Versions::

  • Orm: 4.0.2
  • Dbal: 4.0.2
  • Database: Postgres 13
@mabar mabar added the bug label Jun 2, 2021
@hrach
Copy link
Member

hrach commented Jun 3, 2021

The question is what is the proper fix. I'd rather refrain from depending on some phodoc parser able to handle this 🤔

@mabar
Copy link
Contributor Author

mabar commented Jun 3, 2021

I would say phpstan/phpdoc-parser is reasonable dependency. But not supporting it and just informing by throwing exception might be good enough too.
Entity implementation using real properties and attributes may come with an alternative anyway so the opcache.save_comments could be enabled.

@hrach
Copy link
Member

hrach commented Jun 3, 2021

I would say there are three tasks:

  • throw when invalid type is detected
  • support reading array* and just validating it as an array
  • support validating array against the full type

The validation is rather too complicated and I would refrain from doing so 🤔 Supporting just the parsing could be nicely and easily done without an dependency. I do not expect any other more complex types like functional types and so.

@hrach
Copy link
Member

hrach commented Nov 13, 2021

With the increased amount of what types can be (newly union types in PHP 8.1), we should use PHPStans phpdoc parser.

The question is how the validation should work. Probably full validation should be implemented.

@hrach
Copy link
Member

hrach commented Nov 13, 2021

"Full" validation actually means just:

  • validate first level of intersection & union types
  • no support for generics (including an array generics)
  • no support for array shapes
  • no support for callables
  • validate simple array type (array)
  • validate class/interface types
  • validate primitive types (int, bool, float, string)
  • validate nullable types

@mabar
Copy link
Contributor Author

mabar commented Nov 13, 2021

What will be the behavior for not supported types? Throw exception or note in documentation type may not be true? Exception is safer but it will likely lead to requests for types users actually do use.

@hrach
Copy link
Member

hrach commented Nov 13, 2021

Either:

  • nothing -> you should use static analysis
  • exception that can be muted by modifier (~{typeNotCheckedAtRuntime})

/shrug

@hrach hrach added this to the v5.0 milestone Dec 12, 2021
@hrach hrach removed the feature label Oct 1, 2024
@hrach
Copy link
Member

hrach commented Oct 9, 2024

"Entity property with unknown type is ignored" is not ignored anymore after #680. If the type is too complex, it throws an exception during metadata parsing.

What remains is the better runtime validation or the {typeNotCheckedAtRuntime} approach. I'm keeping this open as it is a needed feature/next step here.

@hrach
Copy link
Member

hrach commented Oct 9, 2024

But removing from 5.0 milestone as the second part will happen later.

@hrach hrach removed this from the v5.0 milestone Oct 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants