TASK: Render menu prototypes without fluid #4623

mficzel · 2023-10-16T20:38:04Z

The prototypes Neos.Neos:Menu, Neos.Neos:DimensionsMenu and Neos.Neos:BreadcrumbMenu are now implemented in pure fusion. The prototype Neos.Neos:MenuItemListRenderer is added that is used by those prototypes to render the result as a very basic unordered list.

The option calculateItemStates is added to the prototypes Neos.Neos:MenuItems, Neos.Neos:DimensionsMenuItems and Neos.Neos:BreadcrumbMenuItems with the default value being false. Once enabled the itemState of each menuItem is calculated. While the calculation of itemState is not overly expensive the calculation depends on the current documentNode and this is only valid for the single document. This makes sharing menu caches between documents harder.

In addition the following issues are fixed:

DimensionMenuItems did not contain a url
MenuItems did not calculate item states other than normal
the signature of NodeAggregateIds::fromNodes(array $nodes) is changed to NodeAggregateIds::fromNodes(Nodes $nodes) and the method
NodeAggregateIds::contain(NodeAggregateId $nodeAggregateId) is added. There were no users of this to be found so this should be safe.
RenderAttributesTrait will now use the value of backedEnums which allows to use Enums in Neos.Fusion:Tag.attributes and Neos.Fusion:Augmenter this was needed for the MenuItemListRenderer

Upgrade instructions

Since the Menus are no longer Templates the templatePathAndFilename was removed together with all state specific attribute rendering properties. You should adjust your rendering accordingly or create custom Menu prototypes that either have a renderer the suits your needs or are based on a Fluid Template again.

Checklist

Code follows the PSR-2 coding style
Tests have been created, run and adjusted as needed
The PR is created against the lowest maintained 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

Neos.Neos/Classes/Fusion/MenuItem.php

mficzel · 2023-10-19T05:36:59Z

Have a question wether we really need the calculateItemState flag if the cost is merely a single findAncestors.

Also the findAncestors could be used for detecting the starting point aswell so we ideally would end up with 2 queries findAncestors and findSubtree for the generation of a menu.

Neos.Neos/Resources/Private/Fusion/Prototypes/MenuItemListRenderer.fusion

The prototypes `Neos.Neos:Menu`, `Neos.Neos:DimensionsMenu` and `Neos.Neos:BreadcrumbMenu` are now implemented in pure fusion. The prototype `Neos.Neos:MenuItemListRenderer` is added that is used by those prototypes to render the result as a very basic unordered list. The flag `calculateItemStates` is added to the prototypes `Neos.Neos:MenuItems`, `Neos.Neos:DimensionsMenuItems` and `Neos.Neos:BreadcrumbMenuItems` with the default value being false. Once enabled the itemState of each menuItem is calculated. In addition the following issues are fixed: - DimensionMenuItems did not contain a url - MenuItems did not calculate item states.

mhsdesign · 2023-10-19T15:08:00Z

Have a question wether we really need the calculateItemState flag if the cost is merely a single findAncestors.

Good point indeed. Hmm we seem to need the ancestors to highlight the trace:

But when we would use findSubtree as you suggested, which we dont do at the moment? The ancestor information would ship free house as its the parent in the tree ... in that case its no expense at all to calculate and we can drop the flag without doubt.

But i think that @bwaidelich is currently creating test cases for the menu so that we can refactor it to use findSubtree - so thats on the list for a next pr.

mficzel · 2023-10-19T16:30:06Z

But i think that @bwaidelich is currently creating test cases for the menu so that we can refactor it to use findSubtree - so thats on the list for a next pr.

@mhsdesign that is a good point and could help to keep this pr in scope. I thought about refactoring this now but it makes more sense with tests.

Regarding the calculateItemState i think it makes sense to have and to disable it by default because those states make sharing of menu caches between documents more "interesting".

mficzel · 2023-10-19T19:55:13Z

But when we would use findSubtree as you suggested, which we dont do at the moment? The ancestor information would ship free house as its the parent in the tree ... in that case its no expense at all to calculate and we can drop the flag without doubt.

I think first should be a findAncestors from currentDocument up. Needed to determine the starting point via entryLevel. From there a single findSubtree should be enough. 2 queries for a menu sounds quite ok to me.

bwaidelich

Great work, this looks and feels so much nicer already! <3

A couple of thoughts (in addition to the - mostly nitpicky – comments):

We should always have large sites in the back of our heads. Even if we only load document ancestor nodes, those can be tens of thousands so we should not load them into memory (Neos already has quite an excessive RAM usage).

So calculating the active state will probably have to be calculated for every item in the menu.

But IMO the bigger issue is that this results in an individual cache entry for every menu for every page which – in turn – means that you'll basically have to flush the whole content cache if you change anything in the menu implementation.

That's why I like the calculateItemState prop and IMO we could even make the @cache configuration depend on it, such that we create a common cache entry if it is disabled

Neos.Neos/Classes/Fusion/AbstractMenuItemsImplementation.php

Neos.Neos/Classes/Fusion/DimensionMenuItem.php

Neos.Neos/Classes/Fusion/MenuItem.php

Neos.Neos/Classes/Fusion/MenuItemsImplementation.php

mficzel · 2023-10-20T08:58:14Z

Short note. After this is merged and with tests for menu items in place we should try to cleanup the dependencies between the MenuItems prototypes (that all are derived from MenuItems).

I would like all *MenuItems prototypes to be standalone fusion prototypes with an implementation class.

bwaidelich · 2023-10-20T09:01:16Z

After this is merged and with tests for menu items in place we should try to cleanup the dependencies between the MenuItems prototypes

Yes, great idea! And we could use that chance to replace the MenuItemState class with a proper enum (and potentially remove the absent case that should IMO never happen)

mficzel · 2023-10-20T09:03:04Z

Absent is a state that can happen in dimension menus. Using enums is a good idea we can aswell implement here.

… nodes This is more memory efficient and we only compare the AggregateIds anyways In addition the signature of `NodeAggregateIds::fromNodes(array $nodes)` is changed to `NodeAggregateIds::fromNodes(Nodes $nodes)` and the method `NodeAggregateIds::contain(NodeAggregateId $nodeAggregateId)` is added. There were no users of this to be found so this should be safe.

Co-authored-by: Bastian Waidelich <[email protected]>

…mState to a BackedEnum In addition the RenderAttributesTrait was adjusted to handle backed enums like stringables.

mficzel · 2023-10-20T16:18:05Z

@bwaidelich i think your issues are addressed. We could think about backporting the backedEnum in attributes support to 8.3 but i would decide upon that later.

bwaidelich · 2023-10-20T16:23:36Z

i think your issues are addressed

@mficzel you're the man, this looks awesome!

Just one last thing for now (and we can also postpone it to a separate PR): Do you think it makes sense to make the @cache config dependent on the calculateItemState flag such that the same cache entry can be shared?

mficzel · 2023-10-20T16:31:01Z

@bwaidelich not safely as entryLevel -x also depends on the current document

would try that separately

bwaidelich

Love it! +1 by reading

Neos.Neos/Resources/Private/Fusion/Prototypes/Menu.fusion

mhsdesign · 2023-10-22T17:14:40Z

I like to wait with merges until Bastians tests are ready ;)

mficzel · 2023-10-22T17:59:44Z

@mhsdesign this changes nothing that was not broken/missing before. Other than the renderers only the state calculation and uris for the dimension menu were added again. By waiting we will only make this more complex by fixing more stuff in a single pr.

bwaidelich · 2023-10-26T08:36:29Z

I like to wait with merges until Bastians tests are ready ;)

For me it's the other way around.
I would suggest to merge this one ASAP so that I can rebase the test-PR on it.
The tests will fail because MenuItemsImplementation still behaves differently than < Neos 9. But we can then start to rewrite it (replacing nasty array juggling with Subgraph queries along the way) in that PR until the tests are green

bwaidelich

There's potentially some follow up work to be done (backports, more tests, cache mode tricks to allow reusability) but this can all be done in separate PRs.

I really love this, thanks for taking care! <3

kitsunet

Thank you! Amazing :)

Neos.Neos/Classes/Fusion/AbstractMenuItemsImplementation.php

Neos.ContentRepository.Core/Classes/SharedModel/Node/NodeAggregateIds.php

Sebobo · 2023-10-26T09:15:23Z

Neos.Neos/Resources/Private/Fusion/Prototypes/DimensionsMenu.fusion

+      documentNode = ${Neos.Caching.entryIdentifierForNode(documentNode)}
+    }
+    entryTags {
+      1 = ${Neos.Caching.nodeTypeTag('Neos.Neos:Document', documentNode)}


Why do we flush on any document change and not just the current one?

Because any document below the starting point could require invalidation.

So descendantOfTag with the startingPoint would be the right choice but that collides with this entry level magic. Also it would invalidate on any content change below.

I really think we should make startingPoint a requirement with default ${site} but that is for another pr.

Sebobo · 2023-10-26T09:16:32Z

Neos.Neos/Resources/Private/Fusion/Prototypes/BreadcrumbMenu.fusion

+      documentNode = ${Neos.Caching.entryIdentifierForNode(documentNode)}
+    }
+    entryTags {
+      1 = ${Neos.Caching.nodeTypeTag('Neos.Neos:Document', documentNode)}


Could we only include the document nodes parents for flushing?

We already flush for all documents

So this way the DimensionsMenu and the Breadcrumb of every site is flushed when any document is changed in the website...

So IMO just including parents and the current node for both cases would solve both and reduce the flushing by a lot

Decided not to do this as this pr is about de-fluidizing not optimizing. Before the prototype inherited the cache annotations of menu which flush for all documents.

There are plans to optimize the menus further so this is only the first step. This pr should not make anything worse but as it is but also should not need to fix more than it promises.

Would like to get this merged to look into the optimizing topic without piling too many changes upon each other.

mhsdesign

I would suggest to merge this one ASAP so that I can rebase the test-PR on it.

yes i agree as discussed ;)

Neos.Neos/Classes/Fusion/AbstractMenuItemsImplementation.php

Sebobo

Fine for now. Just hope the caching topic for this case doesn't get lost. Performance is really important and didn't get enough attention in the past, especially if we want to intrigue our users with the performance of the new CR.

bwaidelich · 2023-10-26T15:03:04Z

Just hope the caching topic for this case doesn't get lost

I agree!
We focused on a 1:1 replacement for now, but the new implementation already does not calculate the item state per menu which will allow for a single reusable cache entry

Adjust rendering to match previous fluid based output. Remove unused fluid template. Relates: neos#4537 Relates: neos#4623

github-actions bot added the 9.0 label Oct 16, 2023

mficzel force-pushed the task/renderMenusWithoutFluid branch 4 times, most recently from 9a9645c to 63c8ea7 Compare October 18, 2023 21:49

ahaeslich reviewed Oct 19, 2023

View reviewed changes

Neos.Neos/Classes/Fusion/MenuItem.php Outdated Show resolved Hide resolved

ahaeslich reviewed Oct 19, 2023

View reviewed changes

Neos.Neos/Resources/Private/Fusion/Prototypes/MenuItemListRenderer.fusion Outdated Show resolved Hide resolved

mficzel force-pushed the task/renderMenusWithoutFluid branch from 63c8ea7 to 9e33add Compare October 19, 2023 07:25

mficzel force-pushed the task/renderMenusWithoutFluid branch from 9e33add to 8e04b9a Compare October 19, 2023 07:29

mficzel marked this pull request as ready for review October 19, 2023 18:20

mficzel requested review from bwaidelich and mhsdesign October 19, 2023 18:20

mficzel changed the title ~~WIP TASK: Render menu prototypes without fluid~~ TASK: Render menu prototypes without fluid Oct 19, 2023

github-actions bot added the Task label Oct 19, 2023

mficzel added the 9.0beta1 label Oct 20, 2023

bwaidelich reviewed Oct 20, 2023

View reviewed changes

mficzel and others added 3 commits October 20, 2023 16:19

Update Neos.Neos/Classes/Fusion/DimensionMenuItem.php

e603c21

Co-authored-by: Bastian Waidelich <[email protected]>

Update Neos.Neos/Classes/Fusion/AbstractMenuItemsImplementation.php

6134527

Co-authored-by: Bastian Waidelich <[email protected]>

mficzel force-pushed the task/renderMenusWithoutFluid branch 2 times, most recently from 850ea62 to d2ebddb Compare October 20, 2023 15:29

TASK: Make MenuItem and DimensionMenuItemReadonly and convert MenuIte…

300f84f

…mState to a BackedEnum In addition the RenderAttributesTrait was adjusted to handle backed enums like stringables.

mficzel force-pushed the task/renderMenusWithoutFluid branch from d2ebddb to 300f84f Compare October 20, 2023 15:37

bwaidelich approved these changes Oct 20, 2023

View reviewed changes

mhsdesign reviewed Oct 21, 2023

View reviewed changes

Neos.Neos/Resources/Private/Fusion/Prototypes/Menu.fusion Show resolved Hide resolved

bwaidelich approved these changes Oct 26, 2023

View reviewed changes

kitsunet approved these changes Oct 26, 2023

View reviewed changes

Sebobo requested changes Oct 26, 2023

View reviewed changes

mhsdesign approved these changes Oct 26, 2023

View reviewed changes

mhsdesign reviewed Oct 26, 2023

View reviewed changes

Neos.Neos/Classes/Fusion/AbstractMenuItemsImplementation.php Outdated Show resolved Hide resolved

Sebobo approved these changes Oct 26, 2023

View reviewed changes

mhsdesign merged commit f271bd3 into neos:9.0 Oct 26, 2023
5 checks passed

ahaeslich mentioned this pull request Nov 5, 2023

TASK: Check and fix Neos:NodeTypes packages #4705

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TASK: Render menu prototypes without fluid #4623

TASK: Render menu prototypes without fluid #4623

mficzel commented Oct 16, 2023 •

edited

Loading

mficzel commented Oct 19, 2023 •

edited

Loading

mhsdesign commented Oct 19, 2023

mficzel commented Oct 19, 2023 •

edited

Loading

mficzel commented Oct 19, 2023 •

edited

Loading

bwaidelich left a comment •

edited

Loading

mficzel commented Oct 20, 2023

bwaidelich commented Oct 20, 2023

mficzel commented Oct 20, 2023 •

edited

Loading

mficzel commented Oct 20, 2023

bwaidelich commented Oct 20, 2023

mficzel commented Oct 20, 2023 •

edited

Loading

bwaidelich left a comment

mhsdesign commented Oct 22, 2023

mficzel commented Oct 22, 2023

bwaidelich commented Oct 26, 2023

bwaidelich left a comment

kitsunet left a comment

Sebobo Oct 26, 2023

mficzel Oct 26, 2023

Sebobo Oct 26, 2023

mficzel Oct 26, 2023

Sebobo Oct 26, 2023 •

edited

Loading

mficzel Oct 26, 2023 •

edited

Loading

mhsdesign left a comment

Sebobo left a comment

bwaidelich commented Oct 26, 2023

TASK: Render menu prototypes without fluid #4623

TASK: Render menu prototypes without fluid #4623

Conversation

mficzel commented Oct 16, 2023 • edited Loading

mficzel commented Oct 19, 2023 • edited Loading

mhsdesign commented Oct 19, 2023

mficzel commented Oct 19, 2023 • edited Loading

mficzel commented Oct 19, 2023 • edited Loading

bwaidelich left a comment • edited Loading

Choose a reason for hiding this comment

mficzel commented Oct 20, 2023

bwaidelich commented Oct 20, 2023

mficzel commented Oct 20, 2023 • edited Loading

mficzel commented Oct 20, 2023

bwaidelich commented Oct 20, 2023

mficzel commented Oct 20, 2023 • edited Loading

bwaidelich left a comment

Choose a reason for hiding this comment

mhsdesign commented Oct 22, 2023

mficzel commented Oct 22, 2023

bwaidelich commented Oct 26, 2023

bwaidelich left a comment

Choose a reason for hiding this comment

kitsunet left a comment

Choose a reason for hiding this comment

Sebobo Oct 26, 2023

Choose a reason for hiding this comment

mficzel Oct 26, 2023

Choose a reason for hiding this comment

Sebobo Oct 26, 2023

Choose a reason for hiding this comment

mficzel Oct 26, 2023

Choose a reason for hiding this comment

Sebobo Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

mficzel Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

mhsdesign left a comment

Choose a reason for hiding this comment

Sebobo left a comment

Choose a reason for hiding this comment

bwaidelich commented Oct 26, 2023

mficzel commented Oct 16, 2023 •

edited

Loading

mficzel commented Oct 19, 2023 •

edited

Loading

mficzel commented Oct 19, 2023 •

edited

Loading

mficzel commented Oct 19, 2023 •

edited

Loading

bwaidelich left a comment •

edited

Loading

mficzel commented Oct 20, 2023 •

edited

Loading

mficzel commented Oct 20, 2023 •

edited

Loading

Sebobo Oct 26, 2023 •

edited

Loading

mficzel Oct 26, 2023 •

edited

Loading