make compatible with Neos 9 #49
Conversation
Benjamin-K
left a comment
Benjamin-K
left a comment
There was a problem hiding this comment.
Thanks for having a look at this. I don't know, if we really need to change the "identifier" property to "storageIdentifier", as the identifier-Property still exists. I think, this is more an issue of "Neos.Form.Builder", as they check for the mixin there, but only use it in form elements (yet) and not in finishers. As this packages finisher still uses the identifier mixin Neos.Form.Builder:IdentifierMixin, this error could by thrown again. Maybe we need to fix it in the Neos.Form.Builder package (possibly here?), but I need to take a closer look at it.
On the other hand: When changing the property, we need to at least provide a node migration, so everybody upgrading from an older version can easily migrate their data to the new property. This is a breaking change we can avoid hopefully.
| $contentGraph = $contentRepository->getContentGraph(WorkspaceName::forLive()); | ||
| $sitesRootNodeNode = $contentGraph->findRootNodeAggregateByType(NodeTypeName::fromString('Neos.Neos:Sites')); | ||
| $contentSubgraph = $contentGraph->getSubgraph( | ||
| DimensionSpacePoint::createWithoutDimensions(), NeosVisibilityConstraints::excludeRemoved() |
There was a problem hiding this comment.
This will lead to different results than we currently have (removedContentShown: true, invissibleContentShown: true, inaccessibleContentShown: true), or am I wrong?
Also we need to use the dimensions provided to this service method, as now they aren't used any more.
There was a problem hiding this comment.
No, you are right for the first point, I should include removed. But despite being called NeosVisibilityConstraint it can only handle removed or disabled content. I don't know why you excludes inaccessibleContent before, but I would include also disabled content then.
And I guess I shot too fast, I'll check that again with the dimensions.
There was a problem hiding this comment.
We tried to find any form in any page or dimension, as the labels are generated "on the fly" when doing an export or viewing the data in the backend. Therefore we also check for removed, hidden or inaccessible content. So adding disabled content would make sense, too.
There was a problem hiding this comment.
I just realised, it gets even more complex, because we need to know in which contentRepository the form is, now I hard coded "default" which works in the project I need it for, but not generically.
There was a problem hiding this comment.
I fixed the dimension handling now - but in my project I only have one dimension, so I didn't test it with multiple dimensions.
And also the contentRepositoryIdentifier is still hardcoded
| $formNode = $q->parents('[instanceof Neos.Form.Builder:NodeBasedForm]')->get(0); | ||
| if (!$formNode instanceof NodeInterface) { | ||
| $formNode = $contentSubgraph->findClosestNode( | ||
| $sitesRootNodeNode->nodeAggregateId, |
There was a problem hiding this comment.
Don't we have to start at the first finisherNode here?
There was a problem hiding this comment.
Yes we have to, but why does it even work like that..!
…tions instead of @return comment, remove comments that don't add value
Yes, but it exists for a different purpose, so IMO it would be cleaner to use a new property than to repurpose the identifier for form elements. And yes, I should have removed the IdentifierMixin, as it isn't needed anymore in my version.
this is a valid point. |
|
Thanks for your updates. I need to have a closer look at it again.
What about a setting, that can be overridden? So "default" can be changed by anybody using this package?
What about keeping the name "identifier" and copying the property from the original mixin? The new Hook fails, because it searches for the |
2 participants
Disclaimer: I only use (and tested) it using the Neos Form Builder
I renamed the property to storageIdentifier, because using identifier as property name led to
because but this this value was already set on creation before the node was persisted. Therefore when the UniqueIdentifierHook tried to get the form, it could not even find the finisherNode in the DB.
The other changes are adaptations to the new content repository and rector reformatings in the yaml files