-
-
Notifications
You must be signed in to change notification settings - Fork 222
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 obsolete findContentStream(s)
#5340
base: 9.0
Are you sure you want to change the base?
TASK: Remove obsolete findContentStream(s)
#5340
Conversation
d0420bc
to
9375fd9
Compare
return $this->getContentRepositoryService($accessorFactory)->dependencies; | ||
} | ||
|
||
final protected function getContentGraphReadModel(): ContentGraphReadModelInterface |
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.
or maybe, if we like it or not, we should just expose the ContentGraphReadModel
ON the content repository directly as @internal
method with note to be careful. We cannot avoid people ditching security (see $context->withoutAuthorisationChecks) and as this ContentGraphReadModel
will soon to be exposed for catchup hooks as api already ...
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.
IMO it's not about preventing folks to intentionally circumvent security – they always can, as you pointed out.
But the API should make that cumbersome or at least very very explicit. And exposing a internal concept in an api-object is a code smell IMO (even if it is marked with warnings)
I think, we should just have some custom "accessor factory" in our CRTestSuiteTrait
that is instantiated once and provides read-access to all required dependencies.
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 instantiated once
Okay agreed, but that escalated to a (possibly) bigger change: #5341 so lets have this optimisation separate for later.
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.
I would rather merge many small changes than always having these huge refactor everyhting things...
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.
oke so are you fine with the code now? @kitsunet
findContentStreams
findContentStream(s)
Actually with the discussion here https://neos-project.slack.com/archives/C04PYL8H3/p1731148458597629 we think that we need some
So in case that we find out that its wrong if the |
As discussed with christian we are rather fine with adding the version on the workspace and push the whole content stream fetching back into the land of internals. There is no need for them to be public in any way. |
see https://neos-project.slack.com/archives/C04PYL8H3/p1730469137241279
these methods were introduced in beta15 to replace the usages of the
@internal
content stream finder in the content stream pruner.With the overhaul of the content stream pruner, we dont need to access all the projections content streams anymore, but it was forgotten to remove these helpers.
For testing we now utilise the same trick as for
countNodes
by acquiring the read model directly.While this requires a work around, we discussed that we are fine with this method
getContentRepositoryServiceFactoryDependencies()
:https://neos-project.slack.com/archives/C04PYL8H3/p1729592139706169?thread_ts=1729581753.563249&cid=C04PYL8H3
Upgrade instructions
Review instructions
Checklist
FEATURE|TASK|BUGFIX
!!!
and have upgrade-instructions