exclude keys in toArray function

[arnedesmedt]

Hi, I would like to add this feature to exclude some keys in the toArray function.

I've already had a few situations where I wanted to convert an ImmutableRecord to an array and always had to unset some keys to use the result as a starting array for another ImmutableRecord.

Are you guys ok to add this feature?

[arnedesmedt]

I'm just thinking right now, that maybe we have to add another function like toArrayWithout because $message->toArray('version', 'name') isn't that self-explanatory. While toArrayWithout is more self-explanatory.

[arnedesmedt]

I've added another commit with a toArrayWithout function

[arnedesmedt]

Sorry for the number of commits, but I was thinking: There is now an blacklist but not a whitelist. So I've added a whitelist and renamed the functions to toArrayOnly and toArrayExcept

[codeliner]

I like the idea. Let me ping some people to collect some more feedback:
What do you think? @sandrokeil @martin-schilling @heiglandreas @jsor @bofalke

[heiglandreas]

I don't really like adding complexity and functionality that can be provided otherwise. In this case the ValueObject includes functionality that allows it to modify it's returned contents according to external requirements. Shouldn't the ValueObject just return it's own data without regards as to what someone else might want to do with it?

Especially as the two functions are really small I would rather see them in an ArrayModifier class that combines those two methods to static function calls. So instead of the current invocation

$newObject = ValueObject::fromArray($state->toArrayOnly('value', 'version'));

you'd get this:

$newObject = ValueObject::fromArray(ArrayModifier::useOnly($state->toArray(), 'value', 'version'));

Of course one could also implement something like this:

$useOnlyValueAndVersion = new ArrayModifierUseOnly('value', 'version');
$newObject = ValueObject::fromArray($useOnlyVaslueAndVersion->fromArray($state->toArray());

Yes, it's a bit more verbose but perhaps also a bit more explicit. And the objects do not need to handle stuff that are not their concern...

[codeliner]

good point @heiglandreas
We could even add this as a suggestion to the docs. What do you think @arnedesmedt Would this be an alternative?

[heiglandreas]

A different approach that would perhaps be even more explicit could be a ValueObjectFactory like this:

$myObject = MyObjectFactory::createFromOtherState($state);

No more fooling around with Array-keys and such on the Object level.

Yes: The MyObjectFactory::createFromOtherState internally does exactly that messy array handling stuff. But it's completely encapsulated into your business logic and (hopefully) excessively tested ;-)

[codeliner]

Regarding testing: as long as an ImmutableRecord is used as source and target object, you don't need excessive testing. ImmutableRecord is super strict on the internal structure. Arrays are only used as transport but as soon as they are passed to the VO, everything is checked: only known properties, missing properties, defautls, null values, correct types.

It is really really hard to do something wrong.
As PHP developers focused on high quality dealing with arrays feels messy, because normally it is, but not when working with ImmutableRecord ;)

But the object factory is definitely a nice way to hide the array stuff. A general purpose ArrayModifier looks a bit more attractive to me personally ;)

[heiglandreas]

I would still test whether everything works as expected to immediately see when one of the objects was adapted without adapting the Factory. Because that will break the factory and then cause an Exception that we probably might not want in production... ;-)

[codeliner]

of course automated tests should touch the factory logic. You want those exceptions become visible as early as possible, not in production when it is too late.

[arnedesmedt]

Ok, I'm fine with the idea of an array modifier. But are you ok to add the 2 modifiiers in this package?

[codeliner]

I like the idea, because it simplifies a common task when working with ImmutableRecord. On the other hand it adds complexity to the interface and maybe people start to request more and more helper methods, so we would need to explain them why we merged yours, but won't merge other stuff. So I tend to agree with @heiglandreas that ImmutableRecord should provide the basics and users use whatever they want to as a helper.
I'm 50/50. That's why I've asked some people to give feedback. Let's wait if we get some more. This is not a decision I want to make alone.

[codeliner]

Oh and BTW: you can always extend ImmutableRecord interface and ImmutableRecordLogic trait to add custom stuff. For example in our current project we added two methods: fromArraySnakeCase() and toArraySnakeCase() because our customer has defined a API contract rule that properties have to be snake case in HTTP requests and responses.
You add the logic once and then use your custom trait and interface in all VOs. That's it.

[martin-schilling]

I am not sure what to think of that either. I agree with @heiglandreas that this kind of functionality is a little to specific to be in the ImmutableRecordLogic and adding this would justify adding a bunch of other functionality too. I do however think that it would be nice for the library to provide this functionality to the user so that not everyone has to manually implement it. Therefore providing additional traits or things like the ArrayModifier or even something like Decorators might be something everyone can live with?

[codeliner]

Providing "add-ons" is actually a good idea! I made a gist to experiment with the idea. This way could provide some helpers and also show people how they can add more helpers themself and/or add more of these "add-ons" in the future, always as a opt-in option and not the other way round. What do you think?

[martin-schilling]

Thats pretty much what I had in mind. Could be somewhat tricky to impmement though specifically what functionality should go where and the naming of these Add-Ons.

[sandrokeil]

@heiglandreas has made some good arguments. Maybe we can simply define plain PHP functions like to_array_except(ImmutableRecord $vo, string ...$excludeKeys) and to_array_only(ImmutableRecord $vo, string ...$includeKeys) because they are universally usable. No interfaces needed.

[codeliner]

@sandrokeil good point. I'm fine with functions as well. What do we do regarding PHP not supporing function autoloading? Schould we add the files to composer.json so that they are always loaded?

[codeliner]

Or a new package like event-engine/php-data-addons?

[martin-schilling]

Is there an advantage with using functions over the ArrayModifier or Trait that I am not seeing? Also I personally feel like adding another package just for this is a little overkill, but that is probably a personal preference.

[sandrokeil]

Or a new package like event-engine/php-data-addons?

Good idea. Mezzio uses the term helpers, but addons sounds also reasonable. But we have to discuss if it makes sense to put it to an extra library.

I think in this case, a plain PHP function would be better, because you don't need a trait or interface here. It's simpler. Yes, we have to add the functions file to the composer.json autoloader (example).

But this solution does no support deep nesting. The snake_case solution needs more code and a trait/interface for deep nesting support or maybe some internal restructuring of the code for easier handling.

Another idea is to add a plugin system for the serialization / toArray stuff. So you can hook into it and transform the fields like you want. But could be complicated for the deep nesting stuff.

[codeliner]

Oh @sandrokeil good that you mention the deep nesting stuff. You're right, more complex addons require an interface to support transformation of nested ImmutableRecords. We should take that into account even if it is not required for the two simple helper functions discussed here.

I think we can agree that we want to add the addon, so that people can use it.
I totally agree that functions would be much simpler given the current use case, but a combi of trait and interface like shown in my gist could be used for more complicated scenarios in the future.
It would be confusing to provide both: functions for simple stuff and interface + trait for complicated stuff. Addons should always look the same.

maybe some internal restructuring of the code for easier handling.

If someone wants to tackle this, I'd be more than happy. ImmutableRecordLogic is 400 LoC of magic code. It's ugly but works great and saves a lot of time. A refactoring should not hurt performance, though.

Another idea is to add a plugin system for the serialization / toArray stuff.

This feels a way to complicated for ImmutableRecord. The 400 magic LoC are already too much for some developers. We should not turn it into a beast.

So my final vote is for Interface + Trait.

[sandrokeil]

Ok, I‘m also for a new repo with interfaces / traits and two issues:

1. Support for exclude keys
2. Support for snake_case

But we should consider to improve the code first, if it is possible / needed and have support for deep nesting. So we need for the exclude functionality dot notation support (contact.credit_card_number).