Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Identify weird bug with ManyToMany relations #1534

Closed
wants to merge 1 commit into
base: 2.1
from

Conversation

Projects
None yet
5 participants
@soyuka
Copy link
Member

soyuka commented Nov 30, 2017

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.

@soyuka soyuka added the bug label Nov 30, 2017

@norkunas

This comment has been minimized.

Copy link
Contributor

norkunas commented Nov 30, 2017

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

@soyuka

This comment has been minimized.

Copy link
Member Author

soyuka commented Dec 1, 2017

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

@norkunas

This comment has been minimized.

Copy link
Contributor

norkunas commented Dec 1, 2017

Good to know 馃憤

@backbone87

This comment has been minimized.

Copy link

backbone87 commented Dec 1, 2017

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 soyuka force-pushed the soyuka:fix/bug-many-to-many branch from 8e2048d to ef1e4fe Dec 1, 2017

@soyuka

This comment has been minimized.

Copy link
Member Author

soyuka commented Dec 1, 2017

@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

This comment has been minimized.

Copy link

backbone87 commented Dec 1, 2017

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 soyuka force-pushed the soyuka:fix/bug-many-to-many branch from ef1e4fe to 2d1288d Dec 1, 2017

@soyuka

This comment has been minimized.

Copy link
Member Author

soyuka commented Dec 1, 2017

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

That's actually good enough IMO! Thanks!

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

@backbone87

This comment has been minimized.

Copy link

backbone87 commented Dec 1, 2017

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

This comment has been minimized.

Copy link
Contributor

norkunas commented Dec 1, 2017

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

@backbone87

This comment has been minimized.

Copy link

backbone87 commented Dec 1, 2017

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

This comment has been minimized.

Copy link
Contributor

norkunas commented Dec 1, 2017

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

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Dec 1, 2017

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.

@norkunas

This comment has been minimized.

Copy link
Contributor

norkunas commented Dec 1, 2017

thanks :)

@soyuka

This comment has been minimized.

Copy link
Member Author

soyuka commented Dec 1, 2017

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

This comment has been minimized.

Copy link
Member Author

soyuka commented Dec 18, 2017

Closing as wontfix.

@soyuka soyuka closed this Dec 18, 2017

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented Dec 25, 2017

We should definitely create a doc entry regarding this issue.

@bendavies

This comment has been minimized.

Copy link
Contributor

bendavies commented Jun 12, 2018

just had to fix this. +1 for a doc 馃槅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.