Identify weird bug with ManyToMany relations

[soyuka]

Q	A
Bug fix?	yes (will be haha)
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	It's a ticket 🕺
License	MIT
Doc PR	na

Take a ManyToMany relation.

Add 2 elements, then remove the first one by issuing PUT requests

The third request (when you remove the first one) will give you an object instead of an array.

Simplified Behat suite (available in this PR):

  Scenario: Add a DummyCar to the Brand
    Given there is a brand and 5 DummyCar
    And I send a "PUT" request to "/brands/1" with body:
      """
      {"car": ["/dummy_cars/1"]}
      """
    And the JSON should be equal to:
      """
      {
        "@context": "/contexts/Brand",
        "@id": "/brands/1",
        "@type": "Brand",
        "car": [
            "/dummy_cars/1"
        ],
        "id": 1
      }
      """

  Scenario: Add another DummyCar to the Brand
    And I send a "PUT" request to "/brands/1" with body:
      """
      {"car": ["/dummy_cars/1", "/dummy_cars/2"]}
      """
    And the JSON should be equal to:
      """
      {
        "@context": "/contexts/Brand",
        "@id": "/brands/1",
        "@type": "Brand",
        "car": [
            "/dummy_cars/1",
            "/dummy_cars/2"
        ],
        "id": 1
      }
      """

  Scenario: Remove the first car of the relation
    When I add "Content-Type" header equal to "application/ld+json"
    And I send a "PUT" request to "/brands/1" with body:
      """
      {"car": ["/dummy_cars/2"]}
      """
    And the JSON should be equal to:
      """
      {
        "@context": "/contexts/Brand",
        "@id": "/brands/1",
        "@type": "Brand",
        "car": [
            "/dummy_cars/2"
        ],
        "id": 1
      }
      """
      The json is equal to:
      {
          "@context": "\/contexts\/Brand",
          "@id": "\/brands\/1",
          "@type": "Brand",
          "car": {
              "1": "\/dummy_cars\/2"
          },
          "id": 1
      } (Behat\Mink\Exception\ExpectationException)

I suspect the denormalization but I might be wrong.

[norkunas]

I have also encountered this.. currently just made a normalizer for doctrine collection which resets indexes 😞

[soyuka]

Wow the dirty fix haha, this needs a proper fix I'll investigate!

[norkunas]

Good to know 👍

[backbone87]

I would almost say that this is a bug in doctrine. If I have a too-many relation with no IndexBy then it should be a classical list and lists can not have holes.

edit: one could argue that this is a known limitation of doctrine collections and the removeCar method should be implemented properly to address this.

[soyuka]

@backbone87 It's just that when removing the value at index 0 the ArrayCollection starts at 1.
Not sure it's a Doctrine bug because the process of re-indexing might be hard to implement. Also I think it may increase the complexity of removing the relations in database.

Also, it may be considered as a symfony bug because when you have [1 => 'Foo'] it'll be JSON encoded as {1: 'Foo'} though it's a perfect valid array in PHP no?

Anyway, I found a proper fix where we recreate the array we just keep a zero-based index instead of using the one provided by the loop (because it can be a key).

[backbone87]

I dont like this approach to fix this. Index maps are a thing in JSON-LD and this is the only way to use them currently. https://json-ld.org/spec/latest/json-ld/#data-indexing

php arrays are ordered maps. If you dont make sure they satisfy list/array constraints then you will ofc not get an array in JSON. Using $collection->removeElement($x) may cause these constraints to be violated. So either we request a change in doctrine so that collection automatically reindexes collections without IndexBy or you solve this in the model: getCars(): array { return array_values($this->cars->toArray()); }

[soyuka]

getCars(): array { return $this->cars->getValues(); }

That's actually good enough IMO! Thanks!

I'll keep the test for documentation purpose :).

[backbone87]

That's actually good enough IMO! Thanks!

i didnt even know that there is a getValues on the doctrine collections. i always used array_values

[norkunas]

But using getValues and returning only array we lose ability to use $collection->matching().

[backbone87]

Doctrine does not recommend returning the collection anyway. If you need the matching method you may as well use a repository or add a delegation method to your entity

[norkunas]

Could you point me where I could read about that?:)

[dunglas]

This is a known issue with Doctrine for years, and IIRC it has been marked as won't fix.
What I usually do to prevent it is reindexing the array in my mutators methods.

It also affects FOSRestBundle btw.

[backbone87]

@norkunas http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/working-with-associations.html#association-management-methods (the note under the code example)

[norkunas]

thanks :)

[soyuka]

I didn't make any changes, feel free to merge this to add a test or just close it :). Thanks @backbone87 for pointing me in the right direction!

[soyuka]

Closing as wontfix.

[dunglas]

We should definitely create a doc entry regarding this issue.

[bendavies]

just had to fix this. +1 for a doc 😆