neos
FEATURE: Include fusion from NodeTypes folder
Fusion Files from the NodeTypes folder are now included automatically for the current site package and for all packages that are configured for fusion autoinclusion via setting.
This change enables the colocation of NodeType yaml files with the integration fusion code. That way all relevant information for a NodeType can be found in a single place (given that the presentation is handled seperately).
Review instructions
The inclusion is now bases on the new package:// stream wrapper from the following flow pr.
https://github.com/neos/flow-development-collection/pull/2919
Checklist
- [x] Code follows the PSR-2 coding style
- [x] Tests have been created, run and adjusted as needed
- [x] The PR is created against the lowest maintained branch
- [x] Reviewer - PR Title is brief but complete and starts with
FEATURE|TASK|BUGFIX - [x] Reviewer - The first section explains the change briefly for change-logs
- [ ] Reviewer - Breaking Changes are marked with
!!!and have upgrade-instructions
I wonder wether a nodetypes://Vendor.Site/**/*.fusion would be the a good pattern. However i still think we can decide on that later and now only discuss wether we want autoloading fusion from nodeTypes.
yes i agree that we can later think about how we make this cleaner than by abusing the resource stream wrapper ;)
btw this will result in a merge conflict with https://github.com/neos/neos-development-collection/pull/3839 and i would like to have my pr merged first ^^ and fix this accordingly - would make sense imo
@mhsdesign agree with the other pr, but there is no requirement regarding the order. There will be a merge conflict that has to be resolved but both directions are equally simple
Hey,
When I read the ../ in the resource path, I am wondering whether we open up an avenue of possible exploits; as there are lots of ways one can abuse this.
Thus my gut feeling would be to not rely on this and at some point be able to restrict loading to a base directory; but I would be curious what e.g. @kdambekalns @bwaidelich or @kitsunet say.
All the best, Sebastian
When I read the ../ in the resource path, I am wondering whether we open up an avenue of possible exploits; as there are lots of ways one can abuse this.
We (very briefly) talked about this during the sprint, and I was surprised this does in fact work. Conclusion was this should be made impossible, so we do need some new way to access things from a package that are not in Resources. Something like a package://… stream wrapper maybe.
The interesting fact is that this the ../ is already used like this to read fusion from the node types. This pr does not change that but makes the dirty part less obvious.
Funny thing is that the package:// would have to be implemented in flow and fusion which surprised me aswell. Turns out fusion implements the supported stream wrappers itself and does not use the flow part. (There are reasons)
That is why I suggest to add a working solution now and not rush packages://.
A thing I would not like to see is a ../NodeTypes include in Neos.Demo as this would propagate a pattern we have to break eventually. However this is what is already done in NodeTypes in the wild and we better offer an alternative soon
What could work would be to implement package:// on the fusion side only for now and do the flow pr later when there is a usecase for it. Having prs in sync in two dev dists always is a mess
Turns out fusion implements the supported stream wrappers itself and does not use the flow part.
Which stream wrappers does Fusion implement? I don't know of any…
fusion should work perfectly when the partial cache is disabled -> as shown by the vfs:// in the tests but the parser cache does have a hardcoded references to 'resource'.
this is done for reasons -> as one cannot find out where a stream wrapper points to (eg realpath doesnt work) -> extending this line to also work with package is super easy ^^ https://github.com/neos/neos-development-collection/blob/4482704aeb696c1697afe12acf57b4e816a953a8/Neos.Fusion/Classes/Core/Cache/ParserCache.php#L101
note that the old parser has a much more hard coded dependency to resource and will not work there ,,,,
@mhsdesign I played around further with a package:// stream wrapper and this is how that would look like: https://github.com/mficzel/neos-development-collection/pull/2 (a pr to the branch for this pr)
Kept the stream wrapper in the fusion package for now. Should be moved to flow and tested there obviously.
FYI: I updated the pr to use the package:// stream wrapper from https://github.com/neos/flow-development-collection/pull/2919 ...
A thing to consider would be wether package://.../NodeTypes/**/*.fusion should be included before or after the resource://../Private/Fusion/Root.fusion currently NodeTypes is included afterwards.
IMHO the conclusion should be that NodeTypes shouldn't be a top level folder but reside in Resources, allowing all access to package resources seems like an anti-pattenr. We wouldn't want people to just go and include stuff from anywhere, would we? Having a quarantine folder like Resources/Private makes a lot of sense to me.
Yes I know you could use filesystem functions and do it yourself, but then you are really bypassing the framework. I am not really in love iwth the idea of a package stream wrapper.
just to mention that: NodeTypes is not an arbitrary new idea of this pr
the folder already exists and works for nodetypes config:
https://github.com/neos/neos-development-collection/pull/3332 https://github.com/neos/flow-development-collection/pull/2919#discussion_r991053460
the folder already exists and works for nodetypes config:
I know, I am saying it was not a good idea to put it there. (Note, hindsight obviously, at the point it time and for what it was supposed for it surely is fine) I would rather suggest the breaking change to move it to resources than opening everything up.... But maybe we can find a clever alternative, that I don't know yet.
Actually i like that NodeTypes is first class citizen in Neos-packages as this is the main thing integrators have to adjust. However this should be discussed elsewhere.
Actually i like that NodeTypes is first class citizen in Neos-packages as this is the main thing integrators have to adjust. However this should be discussed elsewhere.
And I would agree from an integrator perspective, so I guess we should think about some way to make both possible without access to everything
Why dont we just include a realpath in the Fusion via the Fusionservice?
(paths starting with / dont work as of right now, but that can be extended (would work super easy with https://github.com/neos/neos-development-collection/pull/3839))
eg: $packageManager->getPackage("Foo.Bar")->getPackagePath() . "/NodeTypes/Root.fusion"
return $this->fusionParser->parseFrom(
FusionSourceCodeCollection::fromArray(array_filter([
...$this->generateNodeTypeDefinitions(),
...$this->prepareAutoIncludeFusion(),
...$this->prepareFusionIncludes($this->prependFusionIncludes, $siteRootFusionPathAndFilename),
// add nodeTypes
FusionSourceCode::fromFile("/var/html/foo/bar/Packages/Application/Foo.Bar/NodeTypes/Root.fusion")
is_readable($siteRootFusionPathAndFilename)
? FusionSourceCode::fromFile($siteRootFusionPathAndFilename)
: null,
...$this->prepareFusionIncludes($this->appendFusionIncludes, $siteRootFusionPathAndFilename),
]))
);
if we need the NodeTypes/**/* behaviour we can create FusionSourceCodeCollection::fromGlopPattern(__DIR__ . "/NodeTypes/**/*")
Why is the behaviour different to the current fusion loading (eg where need a Root.fusion)
with this approach all fusion files are loaded from the NodeTypes folder directly NodeTypes/**/*.fusion
this would be harder to implement via this idea https://github.com/neos/neos-development-collection/pull/3903#issuecomment-1274597011
and its confusing to required a Root.fusion in Resource/Private/Fusion/ but not in NodeTypes/
if someone then creates a Root.fusion in NodeTypes/ all files will be included twice - which could result in odd overrides.
How about such a rule: if there is a Root.fusion in an autoloaded folder (Nodetypes or Private/Fusion) we include that. Otherwise we use globbing.
im not sure ... as this is technically on the implementation (that i planned) not so cool
Also the root.fusion gives you a better idea about includes (which is quite easy already with glob)
how about we go for Root.fusion in the NodeTypes folder with this pr and discuss the auto glob feature separately?
@mhsdesign one drawback of the approach here is that noone with a classic fusion view can load the fusion from NodeTypes other than by using the current approach we do not want to support anymore. May be valid still but we should consider it.
@kitsunet @mhsdesign added the nodetypes stream wrapper here again. Will clean up once we agree on a direction.
- traversing out of the folder is prevented ... tests missing will copy those from package stream wrapper pr
- did not bother to prevent write access because that is just as bad as writing to other package resources
Like this more now as it will allow other fusion integrators to require fusion from nodetypes aswell for whatever they need this for. That way cases that are based on the pure View from the fusion package can use this aswell.
We may also decide to just add the NodeTypes stream wrapper as having to add ‚include nodetypes://Vendor.Site/**/*.fusion‘ in the Root.fusion might be good enough
Just some (mostly nit pick) comments after skimming over this, you're probably on that anyways. @mficzel Thanks a lot of putting so much energy in this in spite of us getting side tracked in discussions <3
I cleaned up the pr and removed the automatic inclusion from the NodeTypes folder as was discussed in the Neos weekly. Now this only introduces the nodetypes:// stream wrapper that can be used in the Root.fusion without adjusting the way fusion autoloading works.
include nodetypes://Vendor.Site/**/*.fusion
@kitsunet @jonnitto @mhsdesign @bwaidelich