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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TASK: Remove transaction closures from projections #5075

Merged
merged 2 commits into from
May 20, 2024

Conversation

bwaidelich
Copy link
Member

@bwaidelich bwaidelich commented May 19, 2024

Removes the many transactional closures from our projections that made the code harder to read and especially made exceptions much harder to debug.

Due to our current architecture (see #4746) all Projection::apply() calls are wrapped in a transaction anyways

Related: #3854

@mhsdesign
Copy link
Member

I guess this fixes our problem #4970 as well?

@mhsdesign
Copy link
Member

Ah and i see #4988 will remove the whole DbalCheckpointStorage and thus my todo comment that would be solved via this pr. But thats oke thanks a lot!!

if ($this->connection->isRollbackOnly()) {
// TODO as described in https://github.com/neos/neos-development-collection/issues/4970 we are in a bad state and cannot commit after a nested transaction was rolled back.
throw new \RuntimeException(sprintf('Failed to update and commit checkpoint for subscriber "%s" because the transaction has been marked for rollback only. See https://github.com/neos/neos-development-collection/issues/4970', $this->subscriberId), 1711964313);
}

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed with hiding whitespace (what i normally dont like to do) but looks absolutely fine.

@mhsdesign
Copy link
Member

Due to our current architecture (see #4746) all Projection::apply() calls are wrapped in a transaction anyways

okay so currently the nested transactions are obsolete? and how will this change with #4988 will there also be one transaction around?

@bwaidelich
Copy link
Member Author

I'll add one beginTransaction call at the start and one commit call to the end of apply

@mhsdesign
Copy link
Member

sounds good but you might want to have input from someone more qualified than a wall (me regarding this topic :D)

@bwaidelich bwaidelich merged commit a20c7fd into 9.0 May 20, 2024
10 checks passed
@bwaidelich bwaidelich deleted the task/remove-transaction-closures branch May 20, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants