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.

[teohhanhui]

@Taluu Well, it's not unprecedented: https://symfony.com/doc/master/cmf/components/routing/chain.html

[dunglas]

Chain is confusing, in Symfony, most (but not all) "chains" stop after the first match. In term of UX, changing the behavior will be more flexible for the end user.

My proposal:

• change the behavior
• document it as a bug fix that can break existing setup in the CHANGELOG
• is some one need the current behavior, we may introduce a new persister that will not break (or introduce a flag). But as nobody seem to use this behavior for now, I prefer to change it now.

[dunglas]

Done in api-platform/core#2175

[ghettovoice]

Hi for all!
I know this is an old issue
But @dunglas I have a related question: currently chain stops after first match only in persist method. Why remove method doesn't has the same behavior?
Thanks!

[tekmi]

Hi!

Actually, I have the same question as @ghettovoice. In my simple case, I wanted to take full control of the deletion process of my User entity.

I have User entity which has many relation to Address entity. I wanted to remove all the Address entities, but keep the User one, untouched. (Well, only some properties a bit cleaned up).

Because this ChainDataPersister calls the default one ApiPlatform\Core\Bridge\Doctrine\Common\DataPersister, my User entity gets deleted automatically.

@dunglas Is there any chance that the remove method of ChainDataPersister could also stop at the first match?

Thank you.

[soyuka]

I guess it could to be consistent, WDYT @api-platform/core-team ?

[tekmi]

In my Symfony 4.1 app, I ended up overwriting the default Chain Data Persister in my services.yml as follows api_platform.data_persister: '@App\DataPersister\UserPersister'. This is enough to meet my needs, so no more wishes from my side :-)

[dunglas]

to me, it's a bug. remove should stop after the first match too

[soyuka]

done will be tagged in next release.

[ghettovoice]

Thanks @soyuka 👍

[sachingk]

How to change the order the DataPersister execution ? Say I have 2 Persister that return true for support function in the Persister. Let's call it P1 and P2.

Now How to control the order of the DataPersister execution ? I want to executed P2 and then P1.

I am currently using ResumableDataPersisterInterface so the chain will not break.