About chained data persisters

[sgomez]

In the documentation I can read this about DataPersisters:

Custom data persisters can be used to do so. A project can include as many data persisters as it needs. The first able to persist data for a given resource will be used.

However in ChainDataPersister class, persist and remove method executes all persisters than supports the entity:

    /**
     * {@inheritdoc}
     */
    public function persist($data)
    {
        foreach ($this->persisters as $persister) {
            if ($persister->supports($data)) {
                $data = $persister->persist($data) ?? $data;
            }
        }

        return $data;
    }

This could be a bug or the documentation is wrong?

[teohhanhui]

A project can include as many data persisters as it needs. The first able to persist data for a given resource will be used.

Just tag your data persister services with api_platform.data_persister.

The ChainDataPersister is for, well, chaining...

[sgomez]

I am using autowiring, so I didn't need to tag it.

My problem is that I have my own data persister for a doctrine entity, so both are running (doctrine data persister and mine).

ChainDataPersister is the default service with id api_platform.data_persister. It receives all persisters, so all are running. I don't know if this is the expected behavior or an error in the documentation (not the first able to persist but all of them).

[teohhanhui]

Oops... Sorry, you're right. The documentation does not match the code.

[dunglas]

Actually... the code looks wrong to me. We should stop at the first persister supporting the class. It allows specializing. If a user wants to save the resource in several persistence layers, it needs to create a custom persister delegating to the inner ones.

I would change this behavior as a bug fix. ping @gorghoa

[gorghoa]

So, we shall disregard this comment api-platform/core#1967 (comment)? (ping @Taluu)

Hence (re)introducing BC break?

If this is really a thing to fix, the only thing to do is revert this PR api-platform/core#2064 and everything should be fine.

[teohhanhui]

Depending on how you look at it: It's a bug, so not a BC break.

[gorghoa]

Still technically a BC break in my opinion, and at the heart of the solution. Maybe an acceptable one: not for me to say (nor will I be personally impacted. ie. I don’t mind what final decision is made). 🤷‍♀️

All I’m saying is that there is probably people who are relying on this “bug”.

[teohhanhui]

We don't cover BC breaks if you're relying on buggy behaviour.

[dunglas]

I'm all for reverting api-platform/core#2064.
WDYT @api-platform/core-team?

[Taluu]

That is still a BC Break. In 2.2, all persisters were called. That wouldn't be the case in 2.3. I remind that this was done only because we wanted to return a different data when persisting.

So this was not a "buggy behavior" as you call it ; adding the possibility to return the data (something new, the same one... doesn't matter) introduced the bc break, which was the point of my comment. So returning only the first supported persisted data is a BC Break.

Disclaimer : I don't have any custom data persisters, but still.

[teohhanhui]

@Taluu It's a bug that's been around since 2.2. The original behaviour was already buggy.

[teohhanhui]

But yeah, I think it's a hard one. Because the documentation was only added later...

[Taluu]

Then IMO the name of the persister for what you describe as "faulty" is not correct. A chain is what it is : a chain that passes the processed data over the next link that supports it, until none are left. It's what @dunglas proposed #540 (comment).

The name is kinda confusing then.