-
-
Notifications
You must be signed in to change notification settings - Fork 224
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
!!! FEATURE: AssetUsage as CatchUpHook #5258
Conversation
Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php
Outdated
Show resolved
Hide resolved
@@ -244,6 +244,7 @@ jobs: | |||
touch ${{ github.workspace }}/Data/DebugDatabaseDumps/keep | |||
|
|||
./flow package:list --loading-order | |||
FLOW_CONTEXT=Testing/Behat ./flow doctrine:migrate --quiet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(how) is that related?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behat tests are not working without this change, as the CatchUpHook expects the doctrine:migrate command to be executed. But it's not when we run the Neos.ContentRepository.BehavioralTests
.
This was not an issue before, as these tests where only relying on the projection tables. But the new CatchUpHook now, gets executed also in these test cases.
https://github.com/neos/neos-development-collection/blob/9.0/.composer.json#L34-L41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is our long standing: you have to do doctrine migrate sometimes problem otherwise it will not work problem that youll fix with the catchup thing as far as i know. see slack: https://neos-project.slack.com/archives/C04PYL8H3/p1727209924914109?thread_ts=1727117857.409749&cid=C04PYL8H3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, true. But we need to be able to enable it for the test cases where we test the catchup hook 🤷♂️
Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php
Outdated
Show resolved
Hide resolved
Neos.Neos/Classes/AssetUsage/CatchUpHook/AssetUsageCatchUpHook.php
Outdated
Show resolved
Hide resolved
Works like a charm, thanks for the hard work!! |
Post merge + 0.5 i just skimmed through most places thats why no +1 xD |
This is an overhaul of the latest "asset usage" implementation which where using a dedicated projection to keep track of asset usages. As this was not handling all necessary events we tried to improve this, but got stuck due to the need of a lot of information of the graph, workspaces, etc (see #5109).
Therfore we decided to try a new approach to use a CatchUpHook instead, which enables us to access all needed information without re-implementing the whole graph logic.
General approach
The package stores the usage of an asset per node and property in a dimension space point. It only stores the usage in the highest/base workspace (usually "live") as long as there are no changes in the same property in a lower/dependent workspace (e.g. user workspace). This allows to get easily an answer to the following questions:
CatchUpHook
The CatchUpHook is keeping track of all changes to node properties referencing an asset. On replay the CatchUpHook is not handling the events for performance reasons.
Indexing Command
The indexing command fills up the index initially. It can also be used to rebuild the index after projection replays or imports of events.
Fixes: #4808 #5084