snowio / magento2-data-model

5 stars 4 forks source link

Use magic methods for simple withers and getters #14

Open nei opened 6 years ago

nei commented 6 years ago

As discussed here https://github.com/snowio/magento2-data-model/pull/13 we should think about using magic method for this getters. having hundreds of lines of code is not really nice.

Lets try a POC

joshdifabio commented 6 years ago

This might be a better solution: https://github.com/Space48/auto-value-php

joshdifabio commented 6 years ago

@nei Note that if you use magic methods you may as well just use associative arrays and ditch the objects all together. The main reasons we used value objects were to 1) eliminate bugs in mapping code and 2) make functionality discoverable in the IDE.

nei commented 6 years ago

Thanks @joshdifabio I will have a look at this it seems really good and well documented. :) My initial idea was to use magic methods just like doctrine uses and try to have autocomplete with just annotations..

joshdifabio commented 6 years ago

@nei That could work too!

alexanderwanyoike commented 5 years ago

Ive been working on a pr that uses an Abstract value object class, and defines the with and get functions of any property that is defined. If the user tries to call get/with on non defined properties they will be met with an error. Currently testing the work on a shipments https://github.com/snowio/magento2-data-model/pull/24 its still a work in progress @qrz-io doesn't like magic methods.

nei commented 5 years ago

I would think it is better than having huge list of getters and setters like this PaymentData.php even though IDE generates them.

The Downside is that we loose the php type hinting. not sure if that is actually an issue in this case since it seems we are always using strings. what you think @joshdifabio

joshdifabio commented 5 years ago

Personally I would use AutoValue, because it was created by Google to solve this problem. It seems like you are in danger of reinventing the wheel here (as I have done so many times in the past), with all the costs that come with this.

Regarding abstract methods vs. doc block annotations; you have to define the properties and their types somehow, you can either do that using PHP language features using AutoValue, e.g.

abstract class PaymentData
{
    abstract function baseAmountAuthorized(): ?string;
    // or the following -- but this means lots of redundant keywords
    abstract function getBaseAmountAuthorized(): ?string;
}

or you can use extralinguistic features like @method annotations.

If you use languages features then you get type safety, plus things like JSON serialization for free because you can use existing libraries, e.g. symfony/serializer.

In many cases you will also want builders, e.g. where a value has many required properties. If you don't implement builders, the alternative is either a) having a constructor with lots of parameters or b) allowing a value to be constructed in an invalid state without required properties. AutoValue makes it fairly easy to generate builders, which are the best solution to this problem.

We are using AutoValue and symfony/serializer to serialize to JSON and this has eliminated most of the boilerplate for us. (You can use the normalization functionality of symfony/serializer to create JSON object-like arrays which is what your toJson() methods do.) No hand-writing equals methods, serialization logic or builders, plus you get type safety and you can make the most of existing tooling and libraries because you're just writing PHP.

Whatever you do I'd suggest using an existing solution to this problem. I personally think Google's approach to this is the best one (AutoValue), which is why we ported it to PHP.

nei commented 5 years ago

Yeh we were trying to get to a simpler solution but loosing the type safety is not really nice. I think it will be worth using the AutoValue. thanks @joshdifabio @alexanderwanyoike