Allow DataPersister to return a new instance instead of mutating #1967

gorghoa · 2018-05-23T20:11:21Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	unit tests are green, bdd are red, master too seems wrecked 🎶
Fixed tickets	#1799
License	MIT
Doc PR	@todo

closes #1799

DataPersisterInterface can now return the result of the persistence call instead of relying on data mutation. Thus allowing persistence layer to deal with immutable data.

At least, this PR breaks the technical detail that was implicitly using objects instance reference as an identifier for an entity, and conceptually I find this a huge win :)

gorghoa · 2018-05-23T20:16:21Z

I hope, I have not forgotten any DataPersisterInterface implementation in the code base. Any hint on some part of the code I may have missed is welcome!

teohhanhui · 2018-05-23T20:17:46Z

src/Bridge/Doctrine/EventListener/WriteListener.php

@@ -58,7 +58,9 @@ public function onKernelView(GetResponseForControllerResultEvent $event)

        switch ($request->getMethod()) {
            case 'POST':
-                $objectManager->persist($controllerResult);
+                $event->setControllerResult(


CS: We usually don't split into multiple lines like this... Just keep it in one line.

I won’t argue on CS, but in this case, given the low visual impact of ??, I fear that relegating the ?? at the end of the line somehow hides what’s going on.

For instance, to me, this is far less self-explanatory than the splitted version.

case 'POST': $event->setControllerResult($this->dataPersister->persist($controllerResult) ?? $controllerResult)); break;

As I said, this will be my only point of view on CS, I wont argue furthermore if you want to stick to the one line version ;)

nevermind, adding the deprecation notice forces me to assign persist result to a temp var anyway, so the one line will be of fine size 👍

teohhanhui · 2018-05-23T20:17:53Z

src/EventListener/WriteListener.php

@@ -50,7 +50,10 @@ public function onKernelView(GetResponseForControllerResultEvent $event)
            case 'PUT':
            case 'PATCH':
            case 'POST':
-                $this->dataPersister->persist($controllerResult);
+                $event->setControllerResult(


CS: We usually don't split into multiple lines like this... Just keep it in one line.

teohhanhui · 2018-05-23T20:19:46Z

src/DataPersister/DataPersisterInterface.php

@@ -33,6 +33,8 @@ public function supports($data): bool;
     * Persists the data.
     *
     * @param mixed $data
+     *
+     * @return object|void Void will not be supported in API Platform 3, an object should always be returned


I think it might be better if we add deprecation errors where this method is called in API Platform?

Added a deprecation notice in the WriteListener, and ItemMutationResolverFactory.

Doing it in ChainDataPersister should be enough. If we deprecate returning void, please remove it from the supported types and only keep it in the comment.

Ok you already made the changes, so keep them :)

Doing it in ChainDataPersister should be enough.

It's not enough because ChainDataPersister is only an implementation which might or might not be used by the user.

gorghoa · 2018-05-23T21:02:01Z

Thanks for your review @teohhanhui 👍 , code updated :)

soyuka · 2018-05-24T07:10:39Z

tests/EventListener/WriteListenerTest.php

+        $dummy->setName('Dummyrino');
+
+        $dummy2 = new Dummy();
+        $dummy2->setName('Dummyferoce');


Dummycorn 🦄 ?

soyuka

👍

soyuka · 2018-05-24T12:23:46Z

@teohhanhui please merge after taking a last quick look :) thanks!

teohhanhui · 2018-05-24T16:46:44Z

@gorghoa Looks like we need a rebase to merge. 👍

see : api-platform#1799

teohhanhui · 2018-05-24T17:16:52Z

Thanks @gorghoa! 🎉

gorghoa · 2018-05-24T19:55:22Z

Thank you all for your kind reviews and making api platform always better! 💪

Taluu · 2018-07-03T10:16:07Z

src/DataPersister/ChainDataPersister.php

@@ -51,7 +51,7 @@ public function persist($data)
    {
        foreach ($this->persisters as $persister) {
            if ($persister->supports($data)) {
-                $persister->persist($data);
+                return $persister->persist($data) ?? $data;


I know it's a bit late, but wouldn't this prevent all persisters to run, and wouldn't this stop at the first persister that supports this data ? what if I need to apply multiple data persisters ?

Good catch... We may return after the loop instead of inside. WYT? (ping @gorghoa)

indeed, nice catch… Thanks @Taluu for your review!

We may return after the loop instead of inside. WYT?

But from which persister shall we return? First one? Last one? Custom strategy based on priority? (somehow, this question was in fact already valid with the former implementation: the last was the winner, but was it “safe” to assume it was the one that should win the refresh battle?).

I’m now ashamed that my implementation causes two BC breaks:

1st, as @Taluu found out, it skips other registered persisters

2nd. It changes the refresher order

Maybe the best option in term of confidence should be not return data from persister but somehow re-trigger a read item operation. At the cost of an extra read query.

afterthoughts:

This may fix everything…

re-assign $data to the previous persister returned value, then return $data outside the loop as @dunglas suggests.

foreach ($this->persisters as $persister) { if ($persister->supports($data)) { $data = $persister->persist($data) ?? $data; } } return $data;

Everything should be as before 🎉

ps. I would personally rather have a recursive function instead of re-assigning $data in the loop, but I’ve the feeling that we could lose some perfs + inconsistent coding style :)

afterthoughts

It was exactly what i've in mind (last win)

Yep, that's what I had in mind too

Here is the PR #2064

Sorry for the inconvenience, hope this had not causing too much troubles for you @Taluu…

On the plus side, tests are now covering this feature :)

gorghoa force-pushed the feat/1799-handle-immutable-resource-on-persist branch from a73fad3 to 1723b86 Compare May 23, 2018 20:13

teohhanhui suggested changes May 23, 2018

View reviewed changes

teohhanhui added the enhancement label May 23, 2018

gorghoa force-pushed the feat/1799-handle-immutable-resource-on-persist branch 3 times, most recently from 435ad99 to 6056ad5 Compare May 23, 2018 21:01

soyuka reviewed May 24, 2018

View reviewed changes

soyuka approved these changes May 24, 2018

View reviewed changes

dunglas approved these changes May 24, 2018

View reviewed changes

teohhanhui force-pushed the feat/1799-handle-immutable-resource-on-persist branch from 6056ad5 to 96a6148 Compare May 24, 2018 16:57

gorghoa added 2 commits May 24, 2018 18:57

Allow DataPersister to return a new instance instead of mutating

41c9f8d

see : api-platform#1799

inline and add deprecation notice

5f050fe

teohhanhui force-pushed the feat/1799-handle-immutable-resource-on-persist branch from 96a6148 to 5f050fe Compare May 24, 2018 16:59

teohhanhui approved these changes May 24, 2018

View reviewed changes

teohhanhui merged commit dcc9736 into api-platform:master May 24, 2018

gorghoa deleted the feat/1799-handle-immutable-resource-on-persist branch May 24, 2018 19:55

dunglas mentioned this pull request Jun 29, 2018

Document the general design (RAD, CQRS, CQS, event sourcing...) and data persisters api-platform/docs#518

Merged

Taluu reviewed Jul 3, 2018

View reviewed changes

gorghoa mentioned this pull request Jul 3, 2018

fix skipped data persisters on persist #2064

Merged

gorghoa mentioned this pull request Jul 18, 2018

About chained data persisters api-platform/docs#540

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow DataPersister to return a new instance instead of mutating #1967

Allow DataPersister to return a new instance instead of mutating #1967

gorghoa commented May 23, 2018 •

edited

Loading

gorghoa commented May 23, 2018

teohhanhui May 23, 2018

gorghoa May 23, 2018 •

edited

Loading

gorghoa May 23, 2018

teohhanhui May 23, 2018

teohhanhui May 23, 2018

gorghoa May 23, 2018

dunglas May 24, 2018

dunglas May 24, 2018

teohhanhui May 24, 2018

gorghoa commented May 23, 2018

soyuka May 24, 2018

soyuka left a comment

soyuka commented May 24, 2018

teohhanhui commented May 24, 2018

teohhanhui commented May 24, 2018

gorghoa commented May 24, 2018

Taluu Jul 3, 2018

dunglas Jul 3, 2018 •

edited

Loading

gorghoa Jul 3, 2018 •

edited

Loading

dunglas Jul 3, 2018

Taluu Jul 3, 2018

gorghoa Jul 3, 2018 •

edited

Loading

Allow DataPersister to return a new instance instead of mutating #1967

Allow DataPersister to return a new instance instead of mutating #1967

Conversation

gorghoa commented May 23, 2018 • edited Loading

gorghoa commented May 23, 2018

Choose a reason for hiding this comment

gorghoa May 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorghoa commented May 23, 2018

Choose a reason for hiding this comment

soyuka left a comment

Choose a reason for hiding this comment

soyuka commented May 24, 2018

teohhanhui commented May 24, 2018

teohhanhui commented May 24, 2018

gorghoa commented May 24, 2018

Choose a reason for hiding this comment

dunglas Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

gorghoa Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gorghoa Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

gorghoa commented May 23, 2018 •

edited

Loading

gorghoa May 23, 2018 •

edited

Loading

dunglas Jul 3, 2018 •

edited

Loading

gorghoa Jul 3, 2018 •

edited

Loading

gorghoa Jul 3, 2018 •

edited

Loading