Hello Spryker Community! We have a hot 🔥 discussion in the Spryker architecture team regarding

Denis Turkov

Hello Spryker Community!

We have a hot 🔥 discussion in the Spryker architecture team
regarding an RFC (PHP discipline), and I would like to ask for your opinion! :star-struck:

Please use your
👍 for supporting the RFC,
👎 for objecting
❓ for “needs clarification”

The RFC you can find in the attachment.
Looking forward to your feedback! 😎

U01A5ARAXP0

I don't see any problem on implementing this change, so why not? In favor! 👍

Chemaclass

I only see benefits with this proposal :thumbsup_all:

UKHD8KTMF

for me is not clear at what level are we supposed to check. RFC is unclear to me. Checking if function returned expected value is a good practice but at least from RFC it is unclear to me how it is related to transfer objects.

Chemaclass

How I see it is that this RFC introduce a new method set*OrFail() and it’s up to the developer and the domain context to use that one or its former. You are not “suppose to (or have to) check” it at certain level, but depending on what do you want to check, you have the possibility of using setFoo() or setFooOrFail(). This might be interesting when setting values that you want to guaranteed that it’s not null , because in such cases you don’t have to do an early null check. What do you think?

Artem Kosatov

Then also method fromArrayOrFail can be added if not all properties have been set

Unknown

@U01AMFX8S31 There is seldom the case that all properties need to exist. Usually transfers have a lot of optional fields always. But there is indeed an issue checking validity of fromArray() generated ones - or more globally: Having a immutable one that has all required (non optional) fields defined for sure. But thats for another day probably 🙂

UKHD8KTMF

Just keep in mind that transfers are part of the contract and projects might have a different need, like field being optional that is required in the core. This severely limits the extendability of Spryker as project will then need to override all the places where setOrFail method is called.

Unknown

@UKHD8KTMF It is only used where the following code requires it not nullable. So your concerns are not relevant here under those conditions.

UKHD8KTMF

It is actually better than require method as this is replacing it.

UKHD8KTMF

If I would design things I would keep DTOs stupid and let the consumer decide what is required or not (or any other logic).

Chemaclass

But… what’s the problem considering setOrFail() as an alternative for the current required() method? I mean, I only see that you want to guaranteed that certain type is pass, otherwise you want to break asap because the data might be inconsistent. In the end, you you mentioned, this is a tool that the consumer will decide where or how to use, or did I miss something?

UKHD8KTMF

setOrFail() is better than required() because it will fail immediately and not randomly in the code when someone call required in the code. This issue of transfer having a random required filed after an update of a module or installing something is very common when updating projects. setOrFail() will not solve the issue because of the way how we create transfers (from entities) with fromArray(). To fix that we would need mappers with explicit field mapping, but then we would lose the convenience. I guess that is a different discussion.

Unknown

yes, fromArray()/toArray() would need an additional param with "required" fields array to check for not nullable or alike if we really want to get rid of require().
Such an RFC and feature could be also added, independently from what this solves right now.

Unknown

public function fromArrayOrFail(array $data, array $requiredList)

What would you think about this to cover the the topic of require() method deprecation fully? If we know certain ones to be required (not null) we could cover it this way even with this fromArray() usage.

Chemaclass

Interesting indeed 🤔 I think this would be good, I don’t see any problem having/using that, actually

Denis Turkov

Thank you, everyone voting!
This change will be introduced shortly! 🙌