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: Split basic fusion prototype files and define defaults #3915

Merged
merged 5 commits into from Oct 24, 2022

Conversation

Sebobo
Copy link
Member

@Sebobo Sebobo commented Oct 10, 2022

Splits the Fusion prototypes in Neos.Neos and Neos.Fusion into separate files and adds all available parameters of the PHP implementations also in Fusion.
This should make it easier for integrators to understand which options are available for each prototype without reading the their PHP implementation code.

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

This makes it easier to navigate to specific prototypes.
Deprecated prototypes are marked and now in a subfolder
This should make it more obvious which fusion parameters are
available for each prototype and inspecting the PHP code should
not be as necessary anymore.
@mhsdesign
Copy link
Member

NOICE! 🎉

@Benjamin-K
Copy link
Contributor

Very nice.
I'd prefer having an example for each of the prototypes, especially for beginners. A beginner could guess what Neos.Fusion:CanRender or Neos.Fusion:Component does, but an example would help a lot, especially if we have it for some of the prototypes.

@mhsdesign
Copy link
Member

+1

but we should be carefull about the single source of truth - maybe generate the docs from the sourcecode?
https://discuss.neos.io/t/documenting-fusion/6056/4?u=marc

@Sebobo
Copy link
Member Author

Sebobo commented Oct 11, 2022

I thought about the examples too, but would add them in follow-up PRs, as they are more work and better done in batches maybe.

@mhsdesign
Copy link
Member

i agree that can be done later. This IS already a big improvement to have all paths of the object written out ;)

@mhsdesign
Copy link
Member

i dont think we should start inline documentation, when this code would not be used for the rendered docs (as thoses two things can easily diverge).

but currently the fusion ast parser ignores any information about the comments - that would need to be changed...

@Benjamin-K
Copy link
Contributor

Then skip for now. We can add a link to the docs later instead of creating full examples inside the fusion files.
This already is a big improvement 👍

@Sebobo Sebobo requested a review from mhsdesign October 13, 2022 10:04
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.

Seems to work ;)

Question:
Is it now api to include only a specific fusion file from Neos.Fusion?

like
include: resources://Neos.Fusion/Private/Fusion/Value.fusion

i would be against promoting that pattern - as it would not allow us to use the pattern of
moving deprecated objects to Deprecated in the future.

but how can we tell the dev to not do that but use include: resources://Neos.Fusion/Private/Fusion/Root.fusion ?

(same goes for Neos.Neos Root.fusion should be "api" everything else not.)

This allows including only the non deprecated prototypes
from other packages if that is desired.
Copy link
Contributor

@Benjamin-K Benjamin-K 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 now.
Is there a plan for the removal of the auto-include of the Deprecated folder in Neos.Fusion?

@mhsdesign
Copy link
Member

yes we often had that plan ^^ it was even proposed to remove them directly next release (that would have been neos 5 ^^)
#2189

so i guess its gonna be 9.0 since we missed it with 8.0

@Benjamin-K
Copy link
Contributor

What about a soft removal in 8.3 by only removing the include – so Root.fusion in Neos.Fusion only includes Prototypes/*.fusion?
That way, developers can include the Deprecated folder if they still use old prototypes and in 9.0 we can remove them completely? So developers will have kind of a pre-warning that they need to adjust their prototypes.

@mhsdesign
Copy link
Member

mhsdesign commented Oct 24, 2022

even if this would be technically breaking - one can easily fix that as you said.
i like this better that completely destoying everything with 9.0 (ESCR is already quite breaky ^^)

... but on the other hand if we could provide a good migration (i think we have already one?) then it should be quite easy to adjust the code and should not require a special soft removal?

@Sebobo
Copy link
Member Author

Sebobo commented Oct 24, 2022

There is a migration and we should target 9.0 for this.
I don't see how those few prototypes could be a problem until then.

@mhsdesign mhsdesign merged commit c424789 into 8.2 Oct 24, 2022
@mhsdesign mhsdesign deleted the task/split-fusion-prototypes branch October 24, 2022 09:54
@mhsdesign
Copy link
Member

we missed renderer in

@Sebobo
Copy link
Member Author

Sebobo commented Oct 31, 2022

Shame shame shame!

@mhsdesign
Copy link
Member

😂😂
noticed it randomly when using your declarations to teach the api of the matcher

too bad i guess fusion will collapse now and burst into fire. Ill fix it once im on my pc ^^

@mhsdesign
Copy link
Member

Ups we may have broke the default error handling in 8.2 https://discuss.neos.io/t/redirect-issue-to-404-not-found-page-in-neos-demo-neos-flow-8-2-0/6169/3?u=marc

… needs investigation ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants