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] Allow to use f:resource path and improve error message on invalid path #1533

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

BenjaminBeck
Copy link
Contributor

No description provided.

Copy link
Member

@NamelessCoder NamelessCoder left a comment

Choose a reason for hiding this comment

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

We have to consider this very, very carefully as it would allow passthrough to public of any file that can be read. The following also has to be taken into consideration:

  • The end goal is to replace the VHS assets
  • They are intended to work in TS as well as in Fluid so changes probably should not be done in VH classes
  • The reason you get this path is likely because of absRefPrefix or other
  • There are ways to refer to a file in a specific extension's resources folder by FILE:EXT: reference
  • It is equally possible to mark the asset as "external" which prevents the absolute path resolving, and as "standalone" which prevents it from being read by PHP

Also a small tip: checking the first character of a string is much easier if you simply compare the strings. Any number of methods will do: if ($string{0} === $char) - if (substr($string, 0, 1) === $char) or strncmp($string, $char, 1). The methods you should not use would include preg matching and strpos/stripos, both of which are far more expensive as they will analyse the entire string instead of an explicit, single character. See for example https://review.typo3.org/#/c/58533/ which fixes this problem in the TYPO3 core.

@BenjaminBeck
Copy link
Contributor Author

You are right about the danger. I was aware of this and therefore kept the condition very specific.

  • I had forgotten absRefPrefix because I haven't changed this configuration for years. But a change in this configuration didn't lead to a change in the paths created by f:resource in my tests with TYPO3 9. 😕

  • Thanks for the tip regarding the string functions. Now that you say it, it's obvious to me too.

  • EXT:myext/Resources/Public/myscript.js works as a path but I didn't want the unnecessary part.

  • I found something better: There is a ViewHelper for it😲: {{v:extension.path.resources(path: 'myscript.js')}. It uses the current extension if no other one is passed!

So I will use the v:extension.path.resources ViewHelper and update the pull request:

  • The changes to the path processing will be removed.
  • The improved error message remains.

For the goal of replacing VHS Assets:
Maybe we could use a RFC proccess similar to EmberJS ( https://github.com/emberjs/rfcs ) ?

Thanks
Benjamin

@NamelessCoder
Copy link
Member

The improved error message remains.

At the very least this has to be converted to a ViewHelper Exception. The bad part is that introducing new exceptions means the next release is a breaking release. I'm sorry to say, but I'm more inclined to leave this as it is now. If there's any way I can motivate or help you pursue the solution that extracts the most useful parts of VHS assets and puts them in an extension that then has a third-party asset management as dependency, you would be fulfilling a wish that began I think 5 years ago. I am completely out of touch with that area so I don't even know what the preferred solution is at the moment...

Regarding RFC, the vision to switch to a third party asset management was made years ago and is part of the greater goal to make VHS about ViewHelpers only, and in the future separate out the universal ViewHelpers to a package that then works with any Fluid-using framework. Though it isn't an immediate goal, this could also mean that the asset ViewHelper package was universal as well - completely possible to use in TYPO3 CMS, Neos, Symfony and so on.

The way I envision it, the API from VHS more or less remains the same - but the assets are then offloaded instead of handled with the AssetService. Once ready, VHS can either remove or alias all the asset related ViewHelpers and code and suggest this new composer package instead.

I have very little in terms of budget but will happily seek some from VHS users for this. I would also speak to the TYPO3 core team about such a thing because it seems quite appropriate and necessary. TYPO3 really doesn't do assets very well ;)

@BenjaminBeck
Copy link
Contributor Author

BenjaminBeck commented Oct 26, 2018

The bad part is that introducing new exceptions means the next release is a breaking release.

I didn´t consider this. But the error I got was also a exception (Screenshot).
So we would basically check for the same condition but ealier and give a more usefull exception.
The checks for $errorLevel and $this->exceptionalErrors have to be added.
But i am not 100% sure that this might nevertheless be a breaking release.

Is there a play when such a release will happen? Maybe we put this asisde until then?

Regarding RFC

My personal plan/wishlist for handling assets does contain things like es6/typescript transpilation, node modules and lazy module loading. So I will at some time switch to a docker based build system in my development environment. AFAIKT this would use babel or webpack.
The resulting file would still need to somehown go into TYPO3. I saw that the core now also uses TypeScript for its JavaScript, they might already have something similar..

2018-10-25 14_41_57-typo3 exception

@NamelessCoder
Copy link
Member

Is there a plan when such a release will happen?

No solid plan but I would at the very least expect it to not happen until it absolutely has to. Since we only release in the most recent major version it means forcing every user to adopt the breaking change or fall behind on bug fixes etc.

Maybe we put this asisde until then?

Definitely, we'll just leave it open - it also helps when users can find these by searching, especially when the thread contains plenty workarounds like this one does ;)

My personal plan/wishlist for handling assets does contain things like es6/typescript transpilation, node modules and lazy module loading.

My hope is that there is a package that provides the API to register and dependency-order such assets - and has an option for plugins to be added that will do the things you mention, or does them natively.

I believe the TYPO3 core's current TypeScript implementation is a build-time feature, not a feature of TYPO3 itself. Even so, it appears to me to be a slightly too opinionated thing to just say "this is your only option in FE". For our use case here I think we can safely ignore TypeScript and aim for a much more generic solution that handles arbitrary assets and supports plugins.

@NamelessCoder NamelessCoder force-pushed the development branch 3 times, most recently from 7b35850 to 50d769b Compare June 2, 2022 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants