AttributeValue __toString() invalid

[peteward]

Since the value is typed this no longer works. You could cast with (string) but this probably won't have the desired affect on date fields so it should probably just be removed?

https://github.com/Sylius/Sylius/blob/master/src/Sylius/Component/Attribute/Model/AttributeValue.php#L75

[peteward]

Although it looks like the new model for attributes is still expecting __toString() to work:

https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/WebBundle/Resources/views/Frontend/Product/_attribute.html.twig#L1

Also this looks broken 'oldhref' so if someone is going to look at this: https://github.com/Sylius/Sylius/blob/master/src/Sylius/Bundle/AttributeBundle/Resources/views/attributeChoice.html.twig#L13

I'm not sure how well tested this is with datetimes, but maybe a twig function is required to render attribute values, because casting like this won't work.

[nakashu]

@peteward oldhref is doing nothing there, its probably some kind of typo.
I fixed it in enhancment PR #3797 but could not get the js test to pass (don't have time to test it in nix where I can run the tests localy)

[peteward]

OK cool, looks like some nice improvements there but doesn't solve the underlying issues around types.

[nakashu]

Sure it was a bit off topic, just for the oldhref thingy

[Zales0123]

@peteward about this file I think it's confusing naming, because attribute here is really a AttributeValue object, so getting its value is 100% correct ;)

This issue is because of preliminary attribute display system: for now, specific attribute type template is sought here and if it does not exist, attribute is rendered with default _attribute.html.twig template. I've named this variable attribute not attributeValue, to avoid duplication of "value" word ({{ attributeValue.value }}). Moreover, in product context it's more natural to operate on attributes rather than on attribute values (despite it's still AttributeValue object), as it's done also on Product model.

Sure, we should discuss how attributes should be rendered - should it be twig extension, or maybe render() function on AttributeType? Nevertheless, this is a case for other PR, however I know that current attributes display method is trivial ;)

[pjedrzejewski]

So I guess we should simply remove the AttributeValue::__toString. In templates we use getValue() and I don't think we select it anywhere in the forms etc. where __toString might be used.

[Zales0123]

Sure, __toString() is already out 😄

[peteward]

Hi Mateuz,

Thanks for linking this through, it makes more sense now. I'm getting exceptions in a FormType due to __toString() but I think this could be due to code for the legacy attributes model clashing with the new model - I will check this tonight.

Perhaps it would be good to wrap this up in a twig filter or function?

                    {% set attributeTemplate = 'SyliusAttributeBundle:Types:' ~ attribute.attribute.type ~ '.html.twig' %}
                    {% include [attributeTemplate, 'SyliusWebBundle:Frontend/Product:_attribute.html.twig'] with {'attribute': attribute} %}

Although surrounding mark-up could be different in some cases, this feels like the basic rendering of the type could be consistent across places that you want to render it?

As for the Attribute-which-is-really-AttributeValue issue... @michalmarcinkowski brought this one up recently. I agree with him that I think this should be renamed on the product object to attributeValue, not attribute. I know that attributeValue.value seems like duplication but this is the way the model works, renaming it to attribute to try and hide this feels like it causes more headaches.

I'll get back to you on my exception - it's probably a problem ay my end then this one can be closed.

[pjedrzejewski]

@peteward I do not recall any place in Sylius where __toString would be called on AttributeValue, we don't have any select forms with values I believe. Let me know if you find the issue, I'll be glad to help!

[peteward]

Yep, problem my end not using the new AttributeValueType form type.

Removing __toString() looks good.