Don't ignore attribute in hydrator if not present in the request about yin HOT 15 CLOSED

Yes it's a good idea to separate the validation and the hydration like @leclairmael suggest it, you can add more validation than simply test if a field is present or not.
I'm testing this library to do that https://github.com/chanmix51/ParameterJuicer

from yin.

They all got a notification about this conversation :) if someone was interested, they could have created an issue in the past one month. And I think we are good for this major release because it contains quite a few BC breaks and I don't really want to have more.

All in all, I became impatient to release v2.0, so I'll do it today.

from yin.

Hi @lunika!

IMHO, ignoring missing attributes is the right behavior for the hydrators.
If you require an attribute you should implement some validation logic for it, but it should be outside of the hydrator code. We have implemented some standard Validator for a project of ours, I could share some details in case you're interested.

Cheers,
M.

from yin.

Well it's a good behaviour but not the more flexible. The hydrator is not a simple mapper that take data and put them in a property, you are free to implement what you want to do with this data, using a callable is a very good idea and I'm a little bit frustating to not do what I want when the property is missing. Implementing a validator on an array is a little bit bothersome, the optionsResolver will be the one I will use to validate an array.

from yin.

Maybe I'm wrong but I think it should be a simple mapper :)

from yin.

Most of time a simple mapper force you to use property access (you must have getters and setters on your class), they use reflection to map request attributes with class atributes. Here we are free to do what we want, you are also free to use an array, that's cool don't you think ? You also can inject all the services you want in the hydrator and use them in each part of the hydrator. You want something simple, use it as it and if you want something more sofisticated you also can.

In my case I only use value object and use them in a command bus. So I can't use object with setters nor reflection or I break the immutability of my object. So I hydrate an array in the hydrator and then use this array to instanciate my value object so I need all the required attributes in my array. I can use a validator you are right, but if I can I would like to execute the callable event if the attribute is missing.

An alternative way can be to introduce a strict mode in the hydrator, the default value will not affect the current behaviour and if you change it you activate the idea I introduce here.

from yin.

Hey @lunika again :)

Sorry for the long delay, I didn't have much time and brain to think about it during the weekend (and I was super enthusiastic about type hinting the whole project and its supporting libraries too :D).

To be honest, I am also dissatisfied with the way hydrators currently work, and I tried to make them more strict but unfortunately my idea would have contradicted to the spec so I had to stop.

That's why I don't use hydrators in my project because I mainly create immutable Data Transfer Objects (whose constructor is their only injection point) from the request parameters. So we have a very similar problem. :)

All in all, I like the concept to indicate if an attribute is present or not, but it also contradicts to the specification (at least for updates):

If a request does not include all of the attributes for a resource, the server MUST interpret the missing attributes as if they were included with their current values. The server MUST NOT interpret missing attributes as null values.

But a strict mode can be still handy for those poor souls who want to resist the specification! :) So depending on the return value of a isStrictMode(): bool method, missing attributes could also be checked in the hydrator.

Another solution could be to define a getMissingAttributeHydrator() method where one could do anything with the missing values.

What do you think about it? Personally, I clearly prefer the last solution: it feels less hackish than the others. But feel free to express your preference :)

UPDATE: After thinking through how a getMissingAttributeHydrator() would look like I'm not sure I prefer it anymore :S

from yin.

I prefer the first solution :-) and the first solution will be faster to execute than the second one I think.

from yin.

I'd like to release Yin 2.0 soon, so I started to think about this problem again. Another solution could be to add a validate(Request $request) abstract method to hydrators where you are able to validate the request as much as you'd like to. The advantage of this solution is that you can check in one go if all the compulsory attributes are present before hydrating the values.

What do you think?

UPDATE: I added experimental support for this feature in my latest commit (5610c59).

from yin.

I read the commit 5610c59 you should maybe use the exceptionFactory in your examples ?

from yin.

I intentionally didn't use the ExceptionFactory, because I think this exception is out of the scope of the spec and thus out of the scope of the framework. But yeah, a ExceptionFactory::createMissingAttributeException() could be added as a convenience method, but I don't know if it worth it.

By the way, the ExceptionFactory was mainly born to be used by Yin - the framework - in order to be able to customize exceptions thrown by itself. So I think one can just throw their custom exceptions - I believe there are rarely more than one instance of the same exception and that's why there's no need for a factory in its original sense.

I close this issue, because I hope the implemented solution fulfilled its need :) But feel free to reopen it!

from yin.

in that case you should maybe remove the exception factory for this method ?

from yin.

Fair point - I've just removed it!

from yin.

@lunika Do you have any feature in mind which should be implemented before the release of v2.0? I think I finished what I wanted. :)

from yin.

You should maybe open a new issue to send a notification to all developers following this library ?
For now I have no idea

from yin.