canton7 / fuelphp-casset

Better asset management library for fuelphp (with minification!)
MIT License
103 stars 29 forks source link

set_js_option doesnt seem to work with group left blank... #9

Closed leeovery closed 12 years ago

leeovery commented 12 years ago

Hey,

Im trying to add the following lines from my common base controller:

Casset::set_js_option('', 'min', false);
Casset::set_js_option('', 'combine', false);

They are enclosed in an IF clause and are only executed if we are in a development environment. I wont want anything minified or combined when working locally.

I have one group at the moment (jquery) and 2 JS files added via Casset::js(); It all works ok when I change the options from config. But when the above lines are left in the code, the following errors appear:

Warning!

ErrorException [ Warning ]: array_push() expects parameter 1 to be array, null given

PKGPATH/casset/classes/casset.php @ line 505:

504:    }
505:    array_push(static::$groups[$type][$group]['files'], $files);
506:    }
Warning!

ErrorException [ Warning ]: array_push() expects parameter 1 to be array, null given

PKGPATH/casset/classes/casset.php @ line 505:

504:    }
505:    array_push(static::$groups[$type][$group]['files'], $files);
506:    }

Hope thats enough info to go on.

Ive tried to see whats going on here but cant figure it out.

My expected result of this is that all JS, whether from the 'global' group or otherwise, will be injected into the markup un-minified and un-combined.

As an aside, if there is a better of way of doing what Im trying to do, Im all ears :)

Thanks. Lee

leeovery commented 12 years ago

As a follow up Ive now decided to use a closure in the casset config to achieve the desired result. In the min and combine config options I simply return from a closure an environment bool.

canton7 commented 12 years ago

Right... Just trying to work out what's going on here.

Currently, set_*_option only applies itself to groups which already exist. The global group doesn't exist until you add something to it (e.g. using Casset::js()), so I suspect some effect of this is what is causing Casset to fall over. I'll do some testing when I get home tonight.

You probably want Casset:set_js_option('*', ...), as this will apply to all groups, instead of just the global group. Again, it won't apply to groups that don't exist yet, but I'll fix that tonight.

My normal approach is to define a constant based on whether I'm in a dev environment or not (you can set this from a .htaccess file with Setenv CONST value), then use this to determine what value is set into the config file. There should be nothing wrong with your approach, though, so I'll fix whatever's breaking tonight.

E.g.

// In your config
'min' => FUEL_DEV ? false : true,
leeovery commented 12 years ago

Thanks.

Im pretty sure I tried the asterix option as "group" too. Infact I used that first but it didnt seem to work with global. I was trying to do this before using Casset::js() so thats probably the issue.

canton7 commented 12 years ago

Yeah. I'll force creating of the global group if it doesn't exist, and make '*' change the class property as well.

leeovery commented 12 years ago

Great stuff :)

canton7 commented 12 years ago

Can you test the feature/set_group_option_fix branch? Thanks

leeovery commented 12 years ago

Sure Ill get on it now.

leeovery commented 12 years ago

sorry need to do something first. Ill check it out later tonight. within a few hours.

canton7 commented 12 years ago

No worries

leeovery commented 12 years ago

Checked and have submitted pull request.

Its based off your feature branch.

Lee Overy

On 03/10/2011 18:21, "Antony Male" reply@reply.github.com wrote:

No worries

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/issues/9#issuecomment-2274560

canton7 commented 12 years ago

Good catch!

Thanks, tidied it up a little (watch those whitespace changes) and merged in. Congrats on your first included commit :)

Does that all work?

leeovery commented 12 years ago

Haha thanks.

I noticed those whitespace diffs. I strip all white space when I save each file using textmate on mac.

I'll check it out in a bit.

Lee Sent from my iPhone

On 3 Oct 2011, at 21:37, Antony Male reply@reply.github.com wrote:

Good catch!

Thanks, tidied it up a little (watch those whitespace changes) and merged in. Congrats on your first included commit :)

Does that all work?

Reply to this email directly or view it on GitHub: https://github.com/canton7/fuelphp-casset/issues/9#issuecomment-2277403

canton7 commented 12 years ago

Netbeans did up until the point where I re-installed it... Oh well. I'll make a dedicated whitespace-fixing commit in a bit, keep all the noise in the same place :P

leeovery commented 12 years ago

All checked and working fine my end. Good stuff. (from your feature branch with my commit)

This is also getting me pretty sharp at Git too which is a good thing :)

I also use GitFlow btw.

leeovery commented 12 years ago

You probably know this but develop is broken at the moment but to this issue.

Sorry if Im stating obvious!

canton7 commented 12 years ago

All merged in :)

Sorry for the delay, was busy with something.