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

Fixes #497 mainUrl didn't contain asset path #509

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

Conversation

JohJohan
Copy link
Contributor

@JohJohan JohJohan commented Jun 26, 2023

Add missing asset path for main_url to correctly load the assets.

Fixes #497

@JohJohan
Copy link
Contributor Author

@helios-ag and @UlrichHP when you have time can you check it?

@UlrichHP
Copy link
Contributor

Hi @JohJohan,
I tried your solution but it is not working for me.

In my case the assets_path is not defined because the bundles folder is located directly in the public directory.

Your solution seems to make it mandatory to have the bundles folder in another folder.

@JohJohan
Copy link
Contributor Author

Hi @JohJohan,
I tried your solution but it is not working for me.

In my case the assets_path is not defined because the bundles folder is located directly in the public directory.

Your solution seems to make it mandatory to have the bundles folder in another folder.

Okay i can look into that thanks for the feedback. Maybe checking if the 'asset_path' is set might be enough to fix it. I will have a look.

@JohJohan
Copy link
Contributor Author

I see default value for 'asset_path' is 'assets'. If i understand you correctly you set it to 'null' value?

@UlrichHP
Copy link
Contributor

I see default value for 'asset_path' is 'assets'. If i understand you correctly you set it to 'null' value?

I didn't have to. I left it commented like in the README - Basic configuration

I think the assets_path option never really worked...
Even in the controller, it seems it is only used to define a prefix param which is not used afterwards.

@JohJohan
Copy link
Contributor Author

JohJohan commented Jun 27, 2023

I see default value for 'asset_path' is 'assets'. If i understand you correctly you set it to 'null' value?

I didn't have to. I left it commented like in the README - Basic configuration

I think the assets_path option never really worked... Even in the controller, it seems it is only used to define a prefix param which is not used afterwards.

@helios-ag can you confirm the I think the assets_path option never really worked? Otherwise we have a option now that we dont support and we should probably remove it.

But i need it because in a project i have my bundle assets in a different folder there not in /assets there in /public/cdn/. So using the asset_path is good to fix that, but actually using the asset_path it will break others that use default like @UlrichHP

What i can do is:

-  $mainUrl = $package->getUrl('/bundles/fmelfinder/js');
+  $path = '/bundles/fmelfinder/js';
+  if ($this->params['assets_path'] !== 'assets') {
+    $path = '/'.$this->params['assets_path'].$path;
+  }
+  $mainUrl = $package->getUrl($path);

So that it wont break it @helios-ag what do you think? Right now like i said it wont work for my project and i pin it to 12.2.1

@UlrichHP
Copy link
Contributor

@JohJohan
This solution works for me too 😄
Maybe you can modify the if loop condition to this : $this->params['assets_path'] !== '/' && $this->params['assets_path'] !== 'assets' since in the README, it is the default value ?

@JohJohan
Copy link
Contributor Author

@JohJohan This solution works for me too smile Maybe you can modify the if loop condition to this : $this->params['assets_path'] !== '/' && $this->params['assets_path'] !== 'assets' since in the README, it is the default value ?

Yeah that is possible but it feels wrong to skip the config parameter, but using it for the default will break it @helios-ag i'm curious what you think about it?

@helios-ag
Copy link
Owner

@johanadivare seems like prefix not used anymore, in this case prefix can be also eliminated from controller too

@JohJohan
Copy link
Contributor Author

@johanadivare seems like prefix not used anymore, in this case prefix can be also eliminated from controller too

Okay yes, but how should i fix loading my assets then? Now with the current code it loads https://x.net/bundles/fmelfinder/js/elfinder.min.js (404) while it should be loading https://x.net/cdn/bundles/fmelfinder/js/elfinder.min.js (200) as i have everything in a cdn folder in my public folder

@Smanst3r
Copy link

sprintf('/%s/bundles/fmelfinder/js', $this->params['assets_path'])
if I set assets_path: '' the result will be //bundles/fmelfinder/js

Is there a reason why it cannot be done without the leading slash?
sprintf('%s/bundles/fmelfinder/js', $this->params['assets_path'])

e.g.
assets_path: /path-to-somethere/where-you-want # /path-to-somethere/where-you-want/bundles/fmelfinder/js
assets_path: '' # /fmelfinder/js

And one more:
the command elfinder:install option --docroot=public_html should be equal to --docroot=<assets_path> according to this PR?

Thanks for clarifying

@JohJohan
Copy link
Contributor Author

JohJohan commented Apr 10, 2024

@Smanst3r First off sorry for late reply!

Is there a reason why it cannot be done without the leading slash?

Your right that isn't necessary and i removed it.

And one more:
the command elfinder:install option --docroot=public_html should be equal to --docroot=<assets_path> according to this PR?

In my project i run:
elfinder:install --docroot public/cdn

Then i have the following config:

fm_elfinder:
  assets_path: '/cdn'

So no the docroot is different than assets_path right?

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.

The assets for elfinder.min.js are giving 404 when using a different asset_path
5 participants