-
-
Notifications
You must be signed in to change notification settings - Fork 31
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 constructor decorator #79
base: 4.4.x
Are you sure you want to change the base?
Conversation
Signed-off-by: Marcel Kempf <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire proxy logic leads to really non-intuitive semantics: the actual use-case needs a better example
Basically I like the idea and the use of the constructor is definitely missing in this component. But why do I have to add strategies for the individual types myself if they are already defined in the constructor of my object and the |
@froschdesign True, the decorator could easily detect scalar types and cast the values accordingly. Didn't think of that and will add it to the code :) |
The way via the constructor would be great, because then all (ugly) workarounds can finally be removed. In userland code and the documentation: $this->setObject(new Post('', '')); public function __construct(
?string $title = null,
?string $artist = null,
array $tracks = []
) {
$this->title = $title;
$this->artist = $artist;
$this->tracks = $tracks;
} https://docs.laminas.dev/laminas-hydrator/v4/strategies/serializable/#example The main problem here is: We have designed our objects for a framework / library. |
@froschdesign With the latest commit the decorator now casts values for scalar parameters into their proper type. The example from the PR description can now also look like this: <?php
class MyObject {
private int $foo;
public function __construct(int $foo) {
$this->foo = $foo;
}
public function getFoo(): int
{
return $this->foo;
}
}
$hydrator = new ConstructorParametersHydratorDecorator(new ClassMethodsHydrator(false));
$myObject = $hydrator->hydrate(['foo' => '42'], new ProxyObject(MyObject::class));
echo $myObject->getFoo(); // 42 |
Signed-off-by: Marcel Kempf <[email protected]>
Signed-off-by: Marcel Kempf <[email protected]>
Signed-off-by: Marcel Kempf <[email protected]>
6c2b99b
to
cd0442a
Compare
Hi @froschdesign, what do you think about the automatic type casting I added? |
Hello, class EmailAddress
{
private string $emailAddress;
public function __construct(string $emailAddress)
{
if ($emailAddress === '') {
throw new ValueError('Email address cannot be empty');
}
$this->emailAddress = $emailAddress;
}
public function getEmailAddress(): string
{
return $this->emailAddress;
}
} With this kind of object the HydrationInterace::hydrate(array $data, object $object) does not make a sense. It should be IMHO interface HydrationInterface
{
public function hydrate(array $data, ?object $object = null): object;
} because my final class EmailAddressHydrator implements HydrationInterface
{
public function hydrate(array $data, ?object $object = null): object
{
return new EmailAddress($data['email_address'] ?? '');
}
} |
Can we discuss the non intuitive logic of this implementation? When having a look at all other hydrator classes we see that they have all in common: they 're easy to use. Personally, I don't understand why this proxy class is needed. A possible constructor property promotion hydrator can work pretty much like the ClassMethodsHydrator class. Here 's a short, easy to understand example ... A quick draft
Bear with me. This is just a raw, quick draft, meant only as an illustration. So, what does this hydrator exactly do? It pretty much works like the ClassMethodsHydrator class. It takes naming strategies into account, because form data like Why no type validation? You have to do that before. That 's the reason input filters where made for. You have to validate your incoming data before validation. That 's not a task of a hydrator. Why no default values? Default values are set anyway when initializing an object and no other data was given. That 's what default values are made for. They are default. Why no validation if the given data matches all non-default parameters? Well, that 's also the job of input filters and validation. In every conceivable scenario, the data must already be filtered and validated as soon as it is transferred to a hydrator. It is not the task of a hydrator to filter or validate the transferred data. If the data does not match the required constructor parameters, it throws an exception anyway. Example
The above shown code results into the expected object structure. ConclusionHydration should be simple and intutive. Sure, there are use cases, that need complex hydrators and delegation. But this should be an exceptional case. Just keep it simple. |
I think that is the reason why this proposal is not moving forward, because there are already libraries that solve the issue of types in a much better way. Example: https://github.com/EventSaucePHP/ObjectHydrator
If the hydrator can use the typed parameters from the constructor, why should I still create an input filter by hand? If we had a CLI command that automatically created an input filter based on an object or builders like in laminas-form, then I would go along with your argument. |
Signed-off-by: Marcel Kempf [email protected]
Description
Hi, I wanted to get rid of setters just for the sake of hydrators without using reflection to write directly to properties, so I wrote this little decorator that uses the constructor to hydrate an object. Works really well in our projects, maybe this could also be interesting to other people.
Small simplified example:
Open question on my end:
For now I added new boolean flags to
CollectionStrategy
andHydratorStrategy
in order to use the newProxyObject
instead of a reflection. Another way could be two new classes, e.g.ProxyObjectCollectionStrategy
andProxyObjectHydratorStrategy
. What do you think about this?Will adjust/add tests for those when that's decided.