Fix : normalize collection

[MLKiiwy]

In certain case collection are normalize into object (json_encode transform array( 1 => 'A' ) into { '1' : 'a' } instead of ['a'].

Here is the bug fix with the corresponding scenario failing on master.

[vincentchalamon]

Don't forget to create a PR on the 1.0 branch

[dunglas]

I've maybe missed something but how can you be sure that the getValues() method exists? Isn't it something specific to Doctrine?

[MLKiiwy]

The getValues method is from Collection interface of symfony. So every collection on symfony have it. It prevent the issue that array collection internal array can be not ordered properly.

If you want we can add :

$values = $attributeValue instanceof Collection ? $attributeValue->getValues() : $attributeValue;

[dunglas]

AFAIK there is no Collection interface in Symfony. Only in Doctrine collection. And this serializer can be used with other kind of \Traversable such as plain old arrays.

[theofidry]

same concern here, we're not always using Doctrine Collections ;)

[sroze]

We could use Doctrine's Collection interface as it's in doctrine/common and it not related/dependent at all on any database-related stuff.

[teohhanhui]

$values = $attributeValue instanceof \Traversable ? iterator_to_array($attributeValue, false) : array_values($attributeValue);

[dunglas]

Can't you fix your entity getter instead (to add the getValues() call)? If I understand well how it works it add a hard dependency to Doctrine Collections to (IMO) fix a Doctrine bug/edge case.

[theofidry]

How do we do when we actually want to use hashmaps? In my case I don't really care as I've never had to use it with DunglasApiBundle, but it would be an issue to completely prevent this use case IMO.

[MLKiiwy]

@theofidry you're right ! I have an issue now with a relation that is indexed with a string so a hashmap ....

So this simple fix can't works ...
@dunglas I suggest to modifiy the vendor php-property-info because the issue is to identify hash and arrays. In php hash == array but in json hash == object and array == array.

In https://github.com/dunglas/php-property-info/blob/master/src/PropertyInfo/Type.php you can identify a collection, but no difference between hash // array.
If we add a difference : isHash() for exemple => we can modify DoctrineExtractor to identify relation indexed by string.

What do you think ?

[vincentchalamon]

@dunglas @theofidry What do you think about @MLKiiwy question ?

[dunglas]

PHP Property Info is deprecated and has been merged into Symfony as the Symfony PropertyInfo Component.

I don't think that this is the responsibility of this component to handle such differences. What about just having smart getter and setters updating array correctly?

[theofidry]

Well, I would say he's suggestion makes sense but would still be tricky because you can still use an hashmap with int values :/ The best thing IMO is to either reset the keys in the array setter (or remover if you have one) or return all the values in the getter (but likely more costly performance wise):

public function setMyArray(array $arr)
{
    $this->myArray = array_values($arr);
}

public function addToMyArray($val)
{
    $this->myArray[] = $val;
}

public function removeFromMyArray($val)
{
    $key = array_search($val, $this->myArray, true);

    if (false === $key) {
        return;
    }

    if (0 === $key) {
        array_shift($this->myArray);

        return;
    }

    unset($this->myArray[$key]);
    $this->myArray = array_values($this->myArray);
}

[sroze]

@MLKiiwy any news about this PR?

[teohhanhui]

@dunglas

Can't you fix your entity getter instead (to add the getValues() call)? If I understand well how it works it add a hard dependency to Doctrine Collections to (IMO) fix a Doctrine bug/edge case.

As I've mentioned in #285 (comment) it's not a "Doctrine edge case". If we know it's a to-many relation we should normalize it to numerically-indexed non-sparse array.

@theofidry

How do we do when we actually want to use hashmaps?

For a relation? You mean like with Doctrine's "indexBy" option? I'd think that's a responsibility of the mapping metadata layer to provide such information...

[dunglas]

Fixed in 2.0.

[teohhanhui]

How was this fixed?

[dunglas]

Maybe I closed to quickly.

[MLKiiwy]

I'm closing this one, need to test in 2.0 if the issue is still here