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

Add functionality for Explicit Parsing #86

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GeoSot
Copy link
Contributor

@GeoSot GeoSot commented Sep 29, 2022

An approach to enforce explicit DataTypes on meta

Please any feedback will be appreciated, and of course any change to fit in the package standards

Todo:

  • tests
  • add documentation

closes #85

Copy link
Collaborator

@frasmage frasmage left a comment

Choose a reason for hiding this comment

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

I'm not sold on the approach here. The handlers are meant to be serializers. Ideally we should match Laravel's cast handlers.

For example, you don't appear to have any handling for cases where the value isn't compatible with the assigned handler, which could be problematic. Would be better to cast the values into whatever format prior to the serialization handler, so that it picks the right one.

@GeoSot
Copy link
Contributor Author

GeoSot commented Oct 6, 2022

I could add a config option there, to choose between fallback handling or throwing an exception.
Laravel way, the fallback seems acceptable, however in cases we want to explicit enforce a specific cast, usually we want to validate and fail.

Would you like an addition like this?

@frasmage
Copy link
Collaborator

frasmage commented Oct 6, 2022

I'm proposing separating the casting and serialization steps. That way you can cast or validate any value into any format, and only after this will the appropriate serializer be selected automatically based on the resulting format, so there shouldn't be any conflicts.

@GeoSot
Copy link
Contributor Author

GeoSot commented Oct 7, 2022

I have in mind two different cases:

  • Let's consider we want a meta value to be always string. So we have two choices:

    • to validate and parse the input before use setMeta ( I think this is your suggestion), and after this, Handlers will do their stuff
    • to use the already existing Handlers that do the same job but force a specific one, and as an extra step, before parse the value to use canHandleValue for validation. Laravel DataCasters are doing a similar job. They just try to parse, and worst case scenario they throw an error
  • Second scenario is that we have:

    • a meta instance, we fill it with an array of integers array<int>
    • another that we put an array of strings `array
    • and a last one, we put an array of objects array<stdObject>
      Already we have the Handlers that try to parse the given value, and we can register new ones.
      However, the first parser which may be able to parse a value, will handle it, so the parsing is left in luck for more complex structures, and depends on only of the handlers sorting. (Ex: Collections of different models)

On both cases, with the existing functionality, each value needs to be parsed twice.

PS: Next Major version, Handlers could implement Castable (edited)

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.

Enforce DataTypes in Meta values
2 participants