[DX][JsonEncoder] Can we find a better name for normalizer/encoder #59288
Comments
|
Indeed, the naming confusing. And that's unfortunate because IMO this is an excellent name if the Serializer component did not exist. But I agree that we must change it for the sake of a better understanding. I think that we can merge the two interface ValueTransformerInterface
{
public function toJsonValue(mixed $phpValue, array $options = []): mixed;
public function toPhpValue(mixed $jsonValue, array $options = []): mixed;
public static function getJsonValueType(): Type;
}WDYT?
|
|
Agreed 👍 I like |
|
I like the shorter method names proposed by @mtarld 👍 . And combining it in one interface sounds like a good idea as well, although I'm not the best person to determine if there is a common use-case for only implementing one direction? |
|
I think that for plenty of use cases, both direction will be required. Maybe we can add a little example in the documentation showing how to create a value transformer that works in one direction only: public function toJsonValue(mixed $phpValue, array $options = []): mixed
{
throw new \BadMethodCallException(sprintf('"%s" cannot convert value to JSON.', self::class));
}Plus, I think I'm going to create the following attribute to replace the class ValueTransformer
{
private \Closure|null $toPhpValue;
private \Closure|null $toJsonValue;
/**
* @param (callable(mixed, array<string, mixed>?): mixed)|(callable(mixed): mixed) $toPhpValue
* @param (callable(mixed, array<string, mixed>?): mixed)|(callable(mixed): mixed) $toJsonValue
*/
public function __construct(
private string|null $serviceId = null,
mixed $toPhpValue = null,
mixed $toJsonValue = null,
) {
if (!$valueTransformer && !($toPhpValue && $toJsonValue)) {
throw new LogicException('#[ValueTransformer] attribute must declare either a $serviceId or $toPhpValue/$toJsonValue callables.');
}
if ($valueTransformer && ($toPhpValue || $toJsonValue)) {
throw new LogicException('#[ValueTransformer] attribute must declare a $serviceId or $toPhpValue/$toJsonValue callables, but not both.');
}
// ...
}
// ...
}Does it sound good to you? |
|
👍 for the proposed 2-way API. Although it's a whole topic, the attribute idea also looks interesting too. |
|
I personally agree on the entire discussion but the merge of the two interfaces: implementing both directions is not always what is needed. Think at the situation in which we only consume APIs, without subitting ant data: why should have I be forced to implement both directions when I only need toPhp? Aldo about the “transformation” I have some perolexities. I imagine a transformation like something that changes the shape of the array resulting from the decodification of the json string. But hydrating an object from a json, to me, seems more like a conversion. My 2 cents. |
Which can be the case. Changing a value at a given position to make it fit the target object’s property and vice versa, which may also imply to change its type, is transformation. That’s also consistent with other places doing the same kind of processing like Form or jolicode/automapper. Alternative suggestions are welcome but going with a more abstract term would be a mistake I think. |
@chalasr , ok... Thank you for your kindly reply.
Remains the forcing of implementing both the two interfaces that is, to me, a useless forcing: as told, implementing both directions is not always required. 8000 |
|
To me, it's ok to have only on interface, as the common use case is to transform a native value to JSON and the other way around. |
To me, Symfony is the best framework because it doesn't assume nothing and permits to do everything in the best way possible.
Yes, it is possible, but is one more method in a class and one more test for the coverage: the class gets "dirty" and I personally don't like this approach (but, as told, is just my opinion): I prefer two different interfaces. Having two different interfaces also permits to use the IDE to know which directions are implemented where, while having only one interface forces to open each file to see if it implementes both directions or only one and which. In very big projects, it will cause a lot of consumed time (think at an appa that, for example, deals with many apis - as one of my apps that connects with social media, ecommerce platforms, etc... many many apis). More, sometimes methods are complex, also if the common enterprise patterns are followed and sometimes is more useful to split the methods in two different classes than having them in one: having two interfaces permits to do this; having only one doesn't. Finally, I also think having only one interface violates the "Single responsibility principle": also if they affer the same flow, serialization and deserialization are two different responsibilities. If you really like the possibility of having only one interface, it could be created an interface that implements both, so we would all be happy anyway.
Yes, but, again, the less is required to be told in the documentation, the better, IMHO. Anyway, my intent was to focus our attention on little details that, ever in my opinion, made Symfony the best framework out there: no magic, no assumptions, clear code. Maybe you don't agree with me and this is perfectly ok: the only thing I can do anyway, is to thank you for this great component :) |
|
@Aerendir, I understand your point, thank you! Actually, I'm not against having two interfaces (I even created two interfaces in the first place). Maybe we can try finding proper and really clear names for these interfaces? Something like |
|
@mtarld , thank you for your understanding 🙏 Maybe a simple |
|
I think I have a good idea. We can have the following interface: interface ValueTransformerInterface
{
public function transform(mixed $value, array $options = []): mixed;
public static function getJsonValueType(): Type;
}And then bind it like we want: #[ValueTransformer(
toNative: FooTransformer::class,
toJson: BarTransformer::class,
])
// or
#[ValueTransformer(
toNative: BazTransformer::class,
])In that way, you can only implement the transformer in the direction you want, and we still have only one kind of service! WDYT? |
|
Yes, this seems a good and clever compromise 💪 Thank you for your patience and for having spent time thinking at a solution 🙏 |
Description
A frequent thing I'm hearing regarding the Serializer is that the diagram in the docs is the most used piece of doc by many developers, as they always forget which direction is normalization, and which is denormalization.
In the new JsonEncoder component, I see we are reusing this name. I think it would be good if we can maybe try finding a different name for this.
On top of the general confusion about the name, it's also a slightly different concept in both components. I think using the same name in 2 components in the same scope can reduce DX as well. For instance, it will probably not be immediately clear what normalizer we talk about in doc articles like How to Create your Custom Normalizer (given we're probably going to document JsonEncoder somewhere in the "Serializer" guides as well). It can also lead to IDE autocompletion suggesting the wrong interface (like what we're discussing for a different class conflict in #58991).
Can we think of a different name for the classes? (e.g. ValueTransformer) And maybe clearly document the direction in the method name (something like
encodedValueToPhpValue()?).cc @mtarld @chalasr
The text was updated successfully, but these errors were encountered: