typeahead-bundle icon indicating copy to clipboard operation
typeahead-bundle copied to clipboard

Fix for multiple option

Open gonzalocelina opened this issue 8 years ago • 5 comments

When multiple child is set to null (line 30) and on line 62 is called child.id That comprobation throws an error. I don't fully understand why it calls for child.id because after I quit this if() the field started working and it does everything that it should do (adds the

    , saves on the DB, list the existing ones if the Entity have them, etc.) PS: Sorry about my bad english I'm a spanish speaker

gonzalocelina avatar Dec 13 '16 16:12 gonzalocelina

I can't remember why that logic check is there, but your changes assume that the entity always has a __toString method, which is not a good assumption.

lifo101 avatar Dec 13 '16 16:12 lifo101

I'm not convinced this is a proper 'fix'. I haven't had any issues using the Type with single or multiple form fields. I'll have to think about it.

lifo101 avatar Dec 13 '16 23:12 lifo101

I don't see how child is even null in your case. when you build your form or your controller route is possibly adding a null entity?

-- Jason

On Tue, Dec 13, 2016 at 7:03 PM, Agustín Houlgrave <[email protected]

wrote:

@ahoulgrave commented on this pull request.

In Resources/views/Form/typeahead.html.twig https://github.com/lifo101/typeahead-bundle/pull/18#pullrequestreview-12808740 :

@@ -56,7 +56,7 @@

{% block entity_typeahead_list_widget %} {% spaceless %}

  • {% if simple %}
  • {% if simple or child == null %} {% set _id, _value, _render = child, child, child %} {% else %} {% set _id, _value, _render = child.id, attribute(child, property ?: 'id'), attribute(child, render) %}

here child var can be null (L31). cannot call any property without asking.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/lifo101/typeahead-bundle/pull/18#pullrequestreview-12808740, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWa6S8nfqPHT3pYwrg1BTfghp6-6RCFks5rHzI2gaJpZM4LL6uT .

lifo101 avatar Dec 14 '16 00:12 lifo101

What about L31? You're setting it to null

ahoulgrave avatar Dec 14 '16 08:12 ahoulgrave

I can confirm having this issue myself. I will not spend much time on it since I just found out "This bundle does not use the newer Twitter/Typeahead.js javascript library" that I thought it was. (I know, RTFM, sorry).

lethak avatar Dec 29 '16 11:12 lethak