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

Prepare for hydration of typed properties as supported by PHP 7.4 #28

Closed
holtkamp opened this issue Mar 6, 2020 · 8 comments
Closed

Comments

@holtkamp
Copy link
Contributor

holtkamp commented Mar 6, 2020

As of PHP 7.4, typed properties are supported. In case typed properties are used for "mapped" classes used during resource hydration as discussed in #20, the property types of the used class should be respected.

For example:

  • a property of type float can not be assigned a returned string value of 1.0
    • By detecting the type of the property, the value can be determined as (float) $value
  • a property of type DateTimeImmutable can not be assigned a returned string value of '2020-01-01T10:59:59.711148+02:00
    • By detecting the type, the value can be determined as new DateTimeImmutable($value)

Therefore, next to the DocumentHydratorInterface, a DocumentPropertyHydrator or DocumentAttributeHydrator interface would be nice to have to use prepare for this functionality to be used here. Note this requires PHP 7.4or higher.

Already have a working approach, just created this issue to get it out of my head and be able to discuss it before spending time on a PR and tests 😉

@kocsismate What do you think?

@kocsismate
Copy link
Member

Hi @holtkamp ,

Interesting idea! However, it seems to be a bit fragile for me :( What happens with interfaces? For example when the property is a DateTimeInterface? Or when the instantiation of the object is not trivial, e.g. when it contains more constructor parameters? We can't even be sure that the constructor is available because it might not be public.

That's why I prefer hydrating Data Transfer Objects - which are simple data bags. I usually map them to Value Objects if some kind of validation is needed, or directly use them for presentation.

If your preferred workflow is different then the best thing in my opinion to do is override the ClassDocumentHydrator::hydrateResourceAttributes() method - but I think this is what you did anyway. :) It might make sense to create a custom hydrator for each resource if the mapping is not trivial.

To be honest I was always sceptical about general solutions for mapping/hydration which are too advanced because they tend to be fragile (in the sense that people will always keep finding features that it doesn't have yet), and I think it's much easier and less complex to do the mapping/hydration manually - for the price of some repetitive work.

@holtkamp
Copy link
Contributor Author

holtkamp commented Mar 22, 2020

Nice observations!

If your preferred workflow is different then the best thing in my opinion to do is override the ClassDocumentHydrator::hydrateResourceAttributes() method - but I think this is what you did anyway. :)

Indeed, and while doing this, I thought: this can be isolated more by using interfaces: so my "only suggestion" was actually this:

Therefore, next to the DocumentHydratorInterface, a DocumentPropertyHydrator or DocumentAttributeHydrator interface would be nice to have

The example with typed properties I gave was an illustration of "why" this would/could be useful.

So "by default" nothing will change, but one can choose to implement a custom DocumentHydrator, which uses a custom DocumentPropertyHydrator/DocumentAttributeHydrator

Code would probably explain the suggestion better, I can try to come up with a PR and then we can discuss the added value there 😄

@kocsismate
Copy link
Member

OK! I'm looking forward for the PR :)

@holtkamp
Copy link
Contributor Author

@kocsismate as you can see the change is small, but opens up the possibility for somebody to write his own AttributeHydrator without the need to use inheritance to specialize ClassDocumentHydrator::hydrateResourceAttributes()

@kocsismate
Copy link
Member

Yep, I like it :) Thanks!

@holtkamp
Copy link
Contributor Author

Thanks for merging, does this justify a new minor release? Then this issue can be closed.

@kocsismate
Copy link
Member

Sure! I'll release it this week :)

@kocsismate
Copy link
Member

kocsismate commented Apr 24, 2020

@holtkamp Release is done :) https://github.com/woohoolabs/yang/releases/tag/2.3.0

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