make compatible with Neos 9

[Kleisli]

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

Warning: Attempt to read property "aggregateId" on null in /var/www/html/Packages/Application/Neos.Form.Builder/Classes/CommandHook/UniqueIdentifierCommandHook.php line 100£

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

[Benjamin-K]

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.

[Benjamin-K]

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.

[Kleisli]

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.

[Benjamin-K]

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.

[Kleisli]

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.

[Kleisli]

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

[Benjamin-K]

Don't we have to start at the first finisherNode here?

[Kleisli]

Yes we have to, but why does it even work like that..!

[Kleisli]

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.

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.

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.

this is a valid point.

[Benjamin-K]

Thanks for your updates. I need to have a closer look at it again.

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.

What about a setting, that can be overridden? So "default" can be changed by anybody using this package?

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.

this is a valid point.

What about keeping the name "identifier" and copying the property from the original mixin? The new Hook fails, because it searches for the Neos.Form.Builder:IdentifierMixin (see here). So this issue should be fixed, if we keep the property but do not use the mixin. This way we also do not have to write a migration.