Move to an immutable API

[theofidry]

I'm not really sure what's the interest of having an addFilter and co. when everything could be added in the constructor.

[fsmeier]

you mean something like a options-array of filters? for my current project i have like 15-20 filters added.

[mjpvandenberg]

The DeepCopy object is a service (as opposed to an entity or value object). Making a service immutable makes no sense and adds no value, imo. You should be able to add filters to a DeepCopy object that is injected (either via constructor or setter) as a dependency into your own application service.

[theofidry]

@mjpvandenberg there is plenty of value for immutable services. The benefits of immutability don't apply only to value objects or entities.

The only thing that changes in this case, is that you know outright all the filters being used as opposed to only appending some filters without knowing what is there before. It's even a bigger of a difference considering that the order of the filters may matter.

[mjpvandenberg]

Services should be stateless, which implies that there's nothing to mutate.

I guess DeepCopy is not a pure service after all, but a combination of a) a definition of how to deep-copy something, and b) the thing doing the deep-copying according to the definition. It may be appropriate to move part (a) into a separate object that uses the Builder design pattern, especially given that

the order of the filters may matter

[theofidry]

Services should be stateless but that's not always the case (e.g. entity managers or repositories). The built-in filters are stateless, but that's also an extension point where anyone can do anything so there's zero guarantee that they will be.

That said my main concern is not the stateless services as the built-in one are. It's more the order of the filters which matters. Forcing to set all the filters at once is an appropriate and simple solution, and instead of adding a setFilter(), you can just remove addFilter() and move that to the constructor altogether, it's simpler.

So to answer the question "Why to move to an immutable API?", I would say because it's less likely to lead to errors for consumers and make it simpler on our end as well (both for the usage and code-wise).

[mnapoli]

I have to agree that removing the setter solves all those questions. And all containers let you inject stuff in the constructor, so I really don't see a point with the setter too.

[mjpvandenberg]

I suppose that it would make things simpler and avoid problems with filter order.

My preference would be to replace the addFilter() method with a setFilters() method, as opposed to constructor-only filter definition. It's conceptually equally sound and it can prevent the minor hassle of having to manage multiple DeepCopy configurations in the container.

May I suggest to also add a getFilters() method to expose the current set of filters, so that consumers may re-create the DeepCopy with some additional filters if needed.

[theofidry]

I don't think you should manage multiple DeepCopy config. What I'm thinking of is more exposing a function like mentioned in https://github.com/myclabs/DeepCopy/issues/69 and consumers on their end can simply define theirs. The DeepCopy instance is not stateless and it's safer to create a new one for each cloning (the overhead is relatively small). So with the filters it would look more like (provided you are overriding everything):

    /**
     * Deep clone the given value.
     *
     * @param mixed $value
     *
     * @return mixed
     */
    function deep_clone($value)
    {
        $useCloneMethod = true;
        $skipUncloneable = true;
        $filters = [
            [new FooFilter(), new FooMatcher()],
        ];
        $typeFilters = [
            [new FooTypeFilter(), new FooTypeMatcher()],
        ];

        $deepCopy = new DeepCopy($useCloneMethod, $skipUncloneable, $filters, $typeFilters);

        return deepCopy->copy($value);
    }

As opposed to right now:

    /**
     * Deep clone the given value.
     *
     * @param mixed $value
     *
     * @return mixed
     */
    function deep_clone($value)
    {
        $useCloneMethod = true;
        $skipUncloneable = true;
        $filters = [
            [new FooFilter(), new FooMatcher()],
        ];
        $typeFilters = [
            [new FooTypeFilter(), new FooTypeMatcher()],
        ];

        $deepCopy = new DeepCopy($useCloneMethod);
        $deepCopy->skipUncloneable($skipUncloneable);

        foreach ($filters as $filter) {
            $deepCopy->addFilter(...$filter);
        }

        foreach ($typeFilters as $typeFilter) {
            $deepCopy->addTypeFilter(...$typeFilter);
        }

        return deepCopy->copy($value);
    }