fuel / core

Fuel PHP Framework - The core of the Fuel v1 framework
http://fuelphp.com
813 stars 345 forks source link

Theme's Asset class does not respect Asset's "url" setting. #1802

Closed rickmacgillis closed 9 years ago

rickmacgillis commented 9 years ago

Problem

When you set the base URL for the Asset class through it's configuration setting, it doesn't function properly when called through the Theme class. Additionally, it doesn't set the theme name through Theme, or asset path through Theme or the Asset class. The latter probably isn't a bug, but it wasn't documented on the FuelPHP site. Both Asset and Theme's Asset instance both set the type directories, though. (Ex. js, css, img)

In order to set the theme directory, one must set it with \Config::set() and unfortunately Theme doesn't retain the updated configuration settings, so Asset is never notified of the "url" value update. In addition, Asset's add_path method doesn't change the URL at all either.

To reproduce the issue: 1.) Set the URL to the base URL (protocol and domain with the trailing slash) 2.) Write your code to call Theme's Asset instance with the add_path method or try to set the URL with the theme's directory via \Config::set('asset.url', $new_url); 3.) Run the script and see that neither option took effect, thus making that setting incompatible with Theme.

More to it than meets the eye: When using Theme's Asset class with the "url" setting, it also breaks any attempts at sending raw JS through Asset even without Theme. As it prefixes the raw code with the URL.

Reproduce it with: 1.) Set the URL with Config as described above or set it manually in the config file. 2.) Run this code: \Asset::js($output, array(), 'raw', true); return \Asset::render('raw', true);

Solution

1.) To work around this issue, set as much of the base URL as you can in your config. 2.) In your script, use \Config::set('asset.url', $url) to add your theme's name dynamically to the URL. 3.) When you start calling off assets for your output, use the raw Asset class instead of Theme's Asset instance. 4.) To get around the issue with Asset now prefixing raw code with the URL, use the faster solution and just manually add the tags you need to wrap the code in and have it display. You can still use Asset to insert code inline from files and add script/link tags for linked-to files.

WanWizard commented 9 years ago

You have tested this with 1.8/develop?

The Theme's asset instance is a normal Asset instance, nothing special. The only difference is that Theme automatically sets asset paths to search, based on the theme location and configuration.

If your assets are on a static host, that should not be a problem, but the path should still adhere to the logic used in themes (so URL/themes/themename/img/...) and Theme will set the path on the Asset instance.

rickmacgillis commented 9 years ago

I didn't try it on the development release as I need to have as stable of a product as possible. On 1.7.2, this is an issue. Also, the last paragraph doesn't work with or without Theme when using the Asset class. I wasn't sure if that was meant as a feature or if it was a bug.

WanWizard commented 9 years ago

v1 isn't really worked on anymore, the develop branch only contains bugfixes. But I checked, none relevant to Asset and Theme, so this needs to be looked at.

rickmacgillis commented 9 years ago

I look forward to version 2, but it looks to be far off in the future for version 2 to have a stable version since it's only in pre-alpha. I hope the team is still on track for an alpha release on January 1st. That should help get the ball rolling, but like I said, I need something stable and dependable for my project right now or I'd help with the alpha stuff.

Thanks for the update and I look forward to an update on this matter. When version 1.8 is released, will it be considered stable, or will the bug fixes be more of a robust nature? Where are the 1.8 sources so that I can take a look at the changelog?

kenjis commented 9 years ago

@CozyLife 1.8/develop is a branch of this repository, and it is developing version of v1.

You can see the change: https://github.com/fuel/core/compare/1.7/master...1.8/develop

WanWizard commented 9 years ago

Is this still an issue? Checked with the current 1.8/develop codebase?

rickmacgillis commented 9 years ago

Yes, it's still an issue on 1.8/develop.

WanWizard commented 9 years ago

Do you have a code sample to demonstrate it goes wrong? I have trouble understanding what you are trying to do.

Specifiying an asset URL is only useful if you have a separate URL for your asset (in relation to the applications base URL, for example a static asset serving host, or a CDN).

You should not set it to the applications base URL, the presence of the scheme indicates to Asset that the asset to be loaded is not local. I can imagine that including raw javascript is a problem if Asset thinks the js file is on another server, which means it can't read it.

rickmacgillis commented 9 years ago

Setup Try this code using the stock 1.8/develop release's views/welcome/index.php file:

\Theme::instance()->asset->css('bootstrap.css');

Set the asset config's file to have asset.url set to 'http://static.example.com/' for the domain where the asset files are located.

Expected Result The code should output:

<link href="http://static.example.com/default/assets/css/bootstrap.css" rel="stylesheet" />

The default theme's name is 'default' and it should appear as part of the URL so that you can store stylesheets and such for each cosmetically independent theme, along with the other relevant assets.

Actual Result The code will output:

<link href="http://static.example.com/css/bootstrap.css" rel="stylesheet" />

'assets' is the default directory for assets, but it doesn't appear in the URL, and neither does the theme name when in \Theme context.

WanWizard commented 9 years ago

Thanks for the use case, I think I've found it.

The only comment: the expected result in the default config should be: <link href="http://static.example.com/assets/default/css/bootstrap.css" rel="stylesheet" />