Sylius states

[winzou]

Ref: #915
States set according to: https://github.com/Sylius/Sylius/wiki/Status

[pjedrzejewski]

👍

[elliot]

For the sake of consistency, should we make the OrderInterface::STATE_* constants strings as well - instead of integers?

We are already doing it for PaymentInterface, ShipmentInterface, InventoryUnitInterface, and OrderShippingStates.

[winzou]

@elliot I would do the opposite, use integer everywhere? Think of database size once we have lots of inventory units! @Sylius, opinion?

[kayue]

Sylius is using a mix of integer and string, look at the order table...

Personally I vote for string and developer friendliness. It is 2014 we don't have to care about storage anymore.

[winzou]

My concern is not about storage but about execution time for database queries. We very often filter or sort by state, wouldn't it be much faster to do it on integers than strings?
Database queries are already not optimized at all by using Doctrine, if we can do a bit on our side, it's good to take I think.

[kayue]

I cannot deny there will be improvement by using integer but the improvement here is so little and not enough to convince me to give away developer happiness.

[jjanvier]

I vote for strings. Not sure such "optimizations" are relevant.

[winzou]

OK, so 2 vs 1 for now, fair enough. I need more votes for the democracy to win 👯

[antonioperic]

Vote for strings

[stloyd]

I'm always all for integers, as a developer I don't look at value of constant, as it doesn't bother me what's there, the name of that constant should be clear & that's enough.

When I have OrderInterface::STATE_CART = 1 (example), I can easily sort it, I can go with bitwise values if needed etc., when it's OrderInterface::STATE_CART = 'cart', it's not so easy IMO anymore. Additionally there is no point for string as a developer MUST NOT use integer/string values when want to mention the constant value, he MUST call the constant, so in this case: OrderInterface::STATE_CART.

Sorry but I can't understand what's "developer happiness" in this case? You parse your database values manually?! Sorry but I don't get it...

While I'm not totally against the strings, I don't see any pros of using it TBH.

I would vote to leave it as it is, and create new "RFC" about choosing strings or integers for constant values.

[kayue]

My use case is I generate report directly from database, or open database table for debugging purpose. Using string allow me to understand the result without further processing.

[winzou]

But since when we design a database for report & debugging purpose?

[jjanvier]

When you have a bug in production. This bug can't be reproduced on dev. You have no relevant log. So, no other choice than see what's on database.

[pjedrzejewski]

I tend to agree with @jjanvier and @kayue about the strings... However, this should be discussed. Let's open a separate issue.

[winzou]

Storing 1000 times the same string in the same column... My old SQL skills are crying!
Guys, have you really forgotten all database-related common sense? Just for a bit of comfort when debugging your database?

[winzou]

Anyway this PR is green and good to merge :)

[pjedrzejewski]

@winzou This is how I did this before as well, but the main reason was the space. Now it's not that relevant. On the other hand, argument by @stloyd is convincing me, because developer always MUST use the contants and not the raw values, so it does not matter. Yeah, it's easier to browse the database manually without remembering which INT represents which status...

[pjedrzejewski]

This travis false-positives are driving me insane. I want to see green!

[pjedrzejewski]

Thanks Alexandre! 👍

[jjanvier]

@winzou, IMO this is really a detail. Load a big page on Sylius and look at the number of requests. This makes my old SQL skills crying.

And to clarify. I'm totally OK to use integers for constants everywhere. It's no big deal. But if I have the choice between string and int, my first choice goes to int.

Awesome PR anyway :)

[antonioperic]

I agree with @jjanvier - this is 21th century sites are bigger and bigger everyday. we need to do some denormalization. Every new features brings some new queries and if we can do something without JOINS then i think that is good solutions.

I dont say that @stloyd doesn't have right when he is talking about constants but we need to think about performance also

[stloyd]

@pjedrzejewski Additionally I can create a PR that will add "comment" into database field that describes the status int => string.

<field type="string" name="paymentState" column="payment_state" />
<!-- to this -->
<field type="integer" name="paymentState" column="payment_state">
    <options>
        <option name="comment" value="1 = new\n2 = pending\n3 = processing" />
    </options>
</field>

For sure it's supported by the MySQL, not sure about other platforms, but it's not a case IMO =)

btw. Who will create that RFC issue? 💃

[elliot]

The best of both worlds would be to use ENUM's (displayed as strings, stored as uints) - but Doctrine doesn't support those out of the box unless we define our own data type. Do we want to go down that path?