TYPO3.Flow

Patch set 3 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 4 for branch master has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 5 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 6 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 7 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 8 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 9 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 10 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 11 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 12 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 13 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Patch set 14 for branch master of project Packages/TYPO3.Flow has been pushed to the review server.
It is available at https://review.typo3.org/23484

Could you please NOT throw an exception and just log it?! Isn't it a little bit overdone accepting a breaking change just for a developer message?

What about if you wan't to persist the missing entities manually after the controller invocation? In my case I persist user messages entities, which I want to show sometimes also for safe requests.

Hey Marco :)

The Exception is only thrown if all these conditions are met:

- you are running inside the Development Context
- you added Entities to the PersistenceManger
- the request is a safe request which, by default does not persist anything that's added to the PersistenceManager
- you did not trigger persistenceManager->persistAll manually after adding the entities

We had several people quite confused because there changes weren't persisted in safe requests. The only case where
this could be considered "breaking" is where you might have a bug because the changes weren't persisted because of the
safe request.

We choose against a log entry because this would have destroyed the whole point of making novices aware of the
safe request behavior.

If i understand you correctly you have a safe request method, were you change/add entities and persist manually?
then this exception should not be thrown. do you use some kind of customer peristenceManager?

Cheers
Marc

Hey Marc. Thanks for your answer. No I don't have a custom persitence manager. The only thing i did was to persist particular entities (Notifications) after every controller invocation. To realize this behaviour I hooked into the slot AfterControllerInvocation which fits very well.

$dispatcher->connect('TYPO3\Flow\Mvc\Dispatcher', 'afterControllerInvocation', function($request) use($bootstrap) {
$myRepo->persistEntities();
});

But Flow now also uses this slot to throw the discussed exception.

ah, i see. yes that could fail then of course. usually the manual persisting is done right were you did the change
in cases like that.

But Robert introduced another thing quite recently: "Whitelisted objects"

https://review.typo3.org/#/c/28792/

http://docs.typo3.org/flow/TYPO3FlowDocumentation/latest/TheDefinitiveGuide/PartIII/Persistence.html#whitelisted-objects

Maybe that could help you?

Marco Falkenberg wrote:

Hi Marco ;)

Just a little comment regarding:

$dispatcher->connect('TYPO3\Flow\Mvc\Dispatcher', 'afterControllerInvocation', function($request) use($bootstrap) {
$myRepo->persistEntities();
});

I guess you're aware of this, but for anyone else who might just copy that:
With this you circumvent our CSRF-protection. That means: You could change the server state (update, add, delete data) just by clicking a link (in an email for example) if you have the required privileges (e.g. you are logged in to the flow app).
That's because the CSRF token is only checked for "unsafe" requests.

While this might be perfectly fine in your case it's worth mentioning that there are ways around this:

For example you could check the CSRF-token explicitly for the affected actions (and make sure any link contains the token..).
Or you could encapsulate the functionality (in this case: notifications) in some service that persists changes immediately.
Or create those entities asynchronously via AJAX.

It depends on the use case but I usually prefer the last solution because it follows the HTTP spec and it allows for caching(!) - for example if you want to increase a view-counter on each view of a certain article

Marc Neuhaus wrote:

ah, i see. yes that could fail then of course. usually the manual persisting is done right were you did the change
in cases like that.

But Robert introduced another thing quite recently: "Whitelisted objects"

https://review.typo3.org/#/c/28792/

http://docs.typo3.org/flow/TYPO3FlowDocumentation/latest/TheDefinitiveGuide/PartIII/Persistence.html#whitelisted-objects

Maybe that could help you?

That's a clean (white) solution and fits perfect to me needs. Thanks Robert!

Bastian Waidelich wrote:

Marco Falkenberg wrote:

Hi Marco ;)

Just a little comment regarding:

$dispatcher->connect('TYPO3\Flow\Mvc\Dispatcher', 'afterControllerInvocation', function($request) use($bootstrap) {
$myRepo->persistEntities();
});

I guess you're aware of this, but for anyone else who might just copy that:
With this you circumvent our CSRF-protection. That means: You could change the server state (update, add, delete data) just by clicking a link (in an email for example) if you have the required privileges (e.g. you are logged in to the flow app).
That's because the CSRF token is only checked for "unsafe" requests.

While this might be perfectly fine in your case it's worth mentioning that there are ways around this:

For example you could check the CSRF-token explicitly for the affected actions (and make sure any link contains the token..).
Or you could encapsulate the functionality (in this case: notifications) in some service that persists changes immediately.
Or create those entities asynchronously via AJAX.

It depends on the use case but I usually prefer the last solution because it follows the HTTP spec and it allows for caching(!) - for example if you want to increase a view-counter on each view of a certain article

Hi Bastian,

many thanks for this hints and enforcing me to think about more often to follow the HTTP specs :)

Of course I thought about not persisting just everything. While ago I found something like this:

class NotificationRepository {
  ...

  public function persistEntities() {
    foreach ($this->entityManager->getUnitOfWork()->getIdentityMap() as $className => $entities) {
      if ($className === $this->entityClassName) {
        foreach ($entities as $entityToPersist) {
          $this->entityManager->flush($entityToPersist);
        }
        return;
      }
    }
  }
}

Please correct if I'm wrong. If I call NotificationRepository::persistEntities only entities of this certain type will be persisted? That's what I tried in code above. But you're right: at least for this entities the CSRF-protection is circumvented.

FYI: I tried the second solution (persisting immediately) with the mentioned special repository function. But then some changes for other objects get lost. After some debugging I could see why. Look at the last lines of \Doctrine\ORM\UnitOfWork::commit.

In the end I used the new introduced whitelist :)

Thanks anyway!

Marco Falkenberg wrote:

many thanks for this hints and enforcing me to think about more often to follow the HTTP specs :)

You're welcome ;)

If I call NotificationRepository::persistEntities only entities of this certain type will be persisted?

Correct, probably including the dependencies (for example product.category).

FYI: I tried the second solution (persisting immediately) with the mentioned special repository function. But then some changes for other objects get lost.
After some debugging I could see why. Look at the last lines of \Doctrine\ORM\UnitOfWork::commit.

Uh.. ok..

In the end I used the new introduced whitelist :)

Good choice ;)