-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
Move to an immutable API #68
Comments
you mean something like a options-array of filters? |
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. |
@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. |
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
|
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 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). |
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. |
I suppose that it would make things simpler and avoid problems with filter order. My preference would be to replace the May I suggest to also add a |
I don't think you should manage multiple DeepCopy config. What I'm thinking of is more exposing a function like mentioned in #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);
} |
I'm not really sure what's the interest of having an
addFilter
and co. when everything could be added in the constructor.The text was updated successfully, but these errors were encountered: