What are the Slack Archives?

Itโ€™s a history of our time together in the Slack Community! Thereโ€™s a ton of knowledge in here, so feel free to search through the archives for a possible answer to your question.

Because this space is not active, you wonโ€™t be able to create a new post or comment here. If you have a question or want to start a discussion about something, head over to our categories and pick one to post in! You can always refer back to a post from Slack Archives if needed; just copy the link to use it as a reference..

Hello Spryker Community! We have a hot ๐Ÿ”ฅ discussion in the Spryker architecture team regarding

Denis Turkov
Denis Turkov VP Architecture Sprykee Posts: 39 ๐Ÿ› - Council (mod)
edited January 2022 in Slack General

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! ๐Ÿ˜Ž

Comments

  • U01A5ARAXP0
    U01A5ARAXP0 Posts: 119 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    I don't see any problem on implementing this change, so why not? In favor! ๐Ÿ‘

  • Chemaclass
    Chemaclass Tech Lead Spryker Solution Partner Posts: 213 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    I only see benefits with this proposal :thumbsup_all:

  • UKHD8KTMF
    UKHD8KTMF Posts: 393 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    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
    Chemaclass Tech Lead Spryker Solution Partner Posts: 213 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    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
    Artem Kosatov PHP Developer Sprykee Posts: 1 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

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

  • @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
    UKHD8KTMF Posts: 393 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    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.

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

  • UKHD8KTMF
    UKHD8KTMF Posts: 393 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

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

  • UKHD8KTMF
    UKHD8KTMF Posts: 393 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    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
    Chemaclass Tech Lead Spryker Solution Partner Posts: 213 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    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
    UKHD8KTMF Posts: 393 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    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.

  • 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.

  • 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
    Chemaclass Tech Lead Spryker Solution Partner Posts: 213 ๐Ÿง‘๐Ÿปโ€๐Ÿš€ - Cadet

    Interesting indeed ๐Ÿค” I think this would be good, I donโ€™t see any problem having/using that, actually

  • Denis Turkov
    Denis Turkov VP Architecture Sprykee Posts: 39 ๐Ÿ› - Council (mod)

    Thank you, everyone voting!
    This change will be introduced shortly! ๐Ÿ™Œ