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

TASK: Refactor controller to not use static fusion path #2920

Merged
merged 1 commit into from
Jan 15, 2024

Conversation

daniellienert
Copy link
Member

@daniellienert daniellienert commented Jul 5, 2021

The fusion path is now dynamically determined by controller / action
This enables additional actions with separate templates for that controller.

The path Neos.Neos.Ui.BackendController.index is used instead and the initializeView hook with $view->setFusionPath('backend'); was removed.

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good by reading…

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @daniellienert,

Resources/Private/Fusion/Backend/Prototypes/RenderConfiguration.fusion has been moved by this change. That leads to several exceptions like this one:

Could not include Fusion file "resource://Neos.Neos.Ui/Private/Fusion/Prototypes/RenderConfiguration.fusion"
resource://Neos.Neos.Ui/Private/Fusion/Backend/Root.fusion:4

Exception Code | 1347977017
-- | --
Neos\Fusion\Exception
2021072513041021ee63
/tmp/neos/Development/SubContextddev/Cache/Code/Flow_Object_Classes/Neos_Fusion_Core_Parser.php
641
Packages/Application/Neos.Fusion/Classes/Core/Parser.php

This is because it is referenced here:

include: resource://Neos.Neos.Ui/Private/Fusion/Prototypes/RenderConfiguration.fusion

Yet, updating the include won't cut it, because the prototype is also used here:

javascriptBackendInformation = Neos.Neos.Ui:RenderConfiguration {

So, I'd suggest just to move the file back to its original path.

Other than that, the PR looks good :)

After moving back Resources/Private/Fusion/Backend/Prototypes/RenderConfiguration.fusion to Resources/Private/Fusion/Prototypes/RenderConfiguration.fusion, I also recommend rebasing the PR against master. After that, E2E Tests are likely to go green again.

@Sebobo
Copy link
Member

Sebobo commented Jul 29, 2021

Probably also a rebase is needed here to make e2e tests work again.

@mhsdesign mhsdesign self-assigned this Mar 22, 2022
@crydotsnake crydotsnake added the Task Stale Something that would be cool to clean up but is not beeing done label Nov 16, 2022
@markusguenther markusguenther changed the base branch from 8.2 to 8.4 December 28, 2023 16:29
@github-actions github-actions bot added 8.4 and removed 8.2 labels Dec 28, 2023
@markusguenther
Copy link
Member

Also rebased this change and make it work with the latest state.

Copy link
Member

@markusguenther markusguenther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I also adjusted something to make it work ;)

@markusguenther markusguenther requested review from kdambekalns and mhsdesign and removed request for kitsunet December 28, 2023 17:47
@markusguenther markusguenther removed Task Stale Something that would be cool to clean up but is not beeing done Wrong Branch labels Dec 28, 2023
@mhsdesign
Copy link
Member

Im not super sure that the fusion contents are 1 to 1 the same ... aaand we have to remember when upmerging to use the new 9.0 configuration

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.

no yes its the same

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.

But now i fear with the refactoring for 9.0 in #3682 (see commit b5b918d) we have waited a bit too long ^^

i wouldnt want to have different backend controller fusion behaviour for 8.3 and 8.4 and 9.0

so instead i would propose to incorporate this cleanup into the mentioned pr or wait for it to be merged and target 9.0

/ cc @grebaldi

@mhsdesign mhsdesign changed the base branch from 8.4 to 9.0 January 13, 2024 20:45
@github-actions github-actions bot added 9.0 and removed 8.4 labels Jan 13, 2024
@grebaldi
Copy link
Contributor

@mhsdesign:

so instead i would propose to incorporate this cleanup into the mentioned pr or wait for it to be merged and target 9.0

I'd be fine with targetting 9.0, but I would propose to merge this PR first. Then I can rebase #3682 on 9.0 and incorporate the change accordingly.

The fusion path is now dynamically determined by controller / action
This enables additional actions with separate templates for that controller.
@mhsdesign mhsdesign changed the base branch from 9.0 to feature/multi-app/01-foundation January 15, 2024 14:33
@mhsdesign
Copy link
Member

mhsdesign commented Jan 15, 2024

I would propose to merge this PR first

i only agree with this part ;) So i rebased it onto your pr and this pr targets now feature/multi-app/01-foundation its wayyy easier this way imo ^^

If you agree @grebaldi please just hit the merge button - as #3682 is your pr ^^

@mhsdesign mhsdesign marked this pull request as ready for review January 15, 2024 14:35
@grebaldi grebaldi merged commit c5216a0 into feature/multi-app/01-foundation Jan 15, 2024
11 checks passed
@grebaldi grebaldi deleted the task/refactor-controller branch January 15, 2024 14:54
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.

7 participants