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

!!! FEATURE: Separate properties from references in NodeType declaration #4677

Merged
merged 30 commits into from
Apr 6, 2024

Conversation

nezaniel
Copy link
Member

@nezaniel nezaniel commented Nov 1, 2023

Related #4549

Separates property and reference declaration by introducing a dedicated references section:

'My.Package:NodeWithReferences':
   references:
     singleReference:
       constraints:
         maxItems: 1
     multiReferences: {}

The above is a one to one replacement for

'My.Package:NodeWithReferences':
   properties:
     singleReference:
       type: reference
     multiReferences:
       type: references

Upgrade instructions

This pr is made backwards compatible in that the declaration of reference properties are still allowed and will be handled at runtime.
The nodetype contains a breaking change, as the references are not any longer available via getProperties and getPropertyType.
For theses usecases getReferences and hasReference were introduced.

Accessing getPropertyType is unsafe for references and will throw:

$propertyType = $nodeType->getPropertyType($propertyName);
if ($propertyType === 'reference' || $propertyType === 'references') {
    // special stuff
}
// ...

The code must be changed to:

if ($nodeType->hasReference($propertyName)) {
    // special stuff
    break;
}
if ($nodeType->hasProperty($propertyName) {
    $propertyType = $nodeType->getPropertyType($propertyName);
    // ...
}

Existing Neos 9 projects using the legacy property-like declaration syntax AND the new features like reference constraints or properties must be migrated to the new syntax with this change.

Checklist

Ui part: neos/neos-ui#3668

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the 9.0 branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@nezaniel nezaniel marked this pull request as draft November 2, 2023 13:10
@mhsdesign
Copy link
Member

mhsdesign commented Nov 15, 2023

How do we want to continue here? I think i remember that this change caused some trouble in the neos ui, as we need to handle somehow the reference editors.

You proposed this syntax, which i like as it resembles this approach: #4631 (comment) And we already the node type constraint: constraints.nodeTypes

My.NodeType:
  references:
    regularRef: {}
    singleRef:
      constraints:
        valueReference:
          maxValue: 1

Maybe constraints.maxItems or constraints.maxReferences would suit better but thats a detail for now.

About the validation in the commands: We can easily enforce this invariant as the command will not merge the references but replace them. Otherwise we would get into trouble when merging two workspaces. But as said we can simply assert that the to be set references are allowed by the max value.


Regarding the neos ui: We must decide whether it will be easy to implement real references and distinct between properties, or if we should still communicate towards the ui in the legacy format.


Something else i remember from our discussions, which i wanted to document: We want to limit in the nodeType to have either a property or a reference per name. Even-though the escr could technically handle a property and reference with the same name, it makes things in the Ui and other places more complex without real benefit. We discussed to validate this in the construction of the NodeType (initializeNodeType)

@bwaidelich
Copy link
Member

bwaidelich commented Nov 16, 2023

Maybe constraints.maxItems or constraints.maxReferences

I'd prefer maxItems, that's what I suggested with #4631 (comment) (and that was taken from JSON schema and the like)

- added `hasReference`
- __legacyPropertyType used to ensure that the FlowQuery property operation will return the node directly but not an array of nodes
- remove type "reference" or "references" in php doc documentation
- use `hasReference` instead of `getPropertyType === reference`
That way the post processors are forced to be adjusted to work with the new format and will stop working for reference like property types

The inspector.dataTypes.references? configuration was removed as references are special types.
The `NodeReferenceConverter` was removed as it is out of use.
@mhsdesign
Copy link
Member

I got the ui to work again after i continued to split the properties and references. The adjustments for the ui can be found here: neos/neos-ui#3668

@mhsdesign mhsdesign force-pushed the 4549-separateReferencesAndProperties branch from d8dfd02 to c1a3f46 Compare November 17, 2023 09:18
@bwaidelich bwaidelich removed their request for review January 17, 2024 14:50
@mhsdesign
Copy link
Member

While it was noble to use the new reference syntax in all the test files, i fear this is not the right time. With the workspace pr ahead im trying to have as little interference as possible, so i reverted that change via c5b9619. In the end as a followup, when everything is merged we can still cleanup our tests.

@mhsdesign mhsdesign changed the title 4549 - separate properties from references in nodetype declaration !!! FEATURE: Separate properties from references in NodeType declaration Feb 15, 2024
@mhsdesign mhsdesign marked this pull request as ready for review February 15, 2024 12:51
@mhsdesign
Copy link
Member

mhsdesign commented Feb 15, 2024

This is ready to be reviewed :D (in combination with the ui neos/neos-ui#3668 change)
I will update the pr description soon and also check up on the last (optional) todo.

moved to #4981

@mhsdesign mhsdesign force-pushed the 4549-separateReferencesAndProperties branch from 5013a77 to c375d51 Compare March 18, 2024 13:05
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.

Neos ui e2e pass locally and together with the ui adjustments the node creation handle also works

Will merge this now as agreed in the weekly and with bernhard.

@mhsdesign mhsdesign merged commit 7cf6bee into 9.0 Apr 6, 2024
9 checks passed
@mhsdesign mhsdesign deleted the 4549-separateReferencesAndProperties branch April 6, 2024 20:01
@mhsdesign mhsdesign mentioned this pull request Apr 6, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants