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

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

Closed
leeovery opened this issue Oct 3, 2011 · 16 comments
Closed

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

leeovery opened this issue Oct 3, 2011 · 16 comments
Assignees

Comments

@leeovery
Copy link
Contributor

leeovery commented Oct 3, 2011

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
Copy link
Contributor Author

leeovery commented Oct 3, 2011

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
Copy link
Owner

canton7 commented Oct 3, 2011

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
Copy link
Contributor Author

leeovery commented Oct 3, 2011

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
Copy link
Owner

canton7 commented Oct 3, 2011

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

@leeovery
Copy link
Contributor Author

leeovery commented Oct 3, 2011

Great stuff :)

@ghost ghost assigned canton7 Oct 3, 2011
@canton7
Copy link
Owner

canton7 commented Oct 3, 2011

Can you test the feature/set_group_option_fix branch? Thanks

@leeovery
Copy link
Contributor Author

leeovery commented Oct 3, 2011

Sure Ill get on it now.

@leeovery
Copy link
Contributor Author

leeovery commented Oct 3, 2011

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

@canton7
Copy link
Owner

canton7 commented Oct 3, 2011

No worries

@leeovery
Copy link
Contributor Author

leeovery commented Oct 3, 2011

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:
#9 (comment)

@canton7
Copy link
Owner

canton7 commented Oct 3, 2011

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
Copy link
Contributor Author

leeovery commented Oct 3, 2011

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:
#9 (comment)

@canton7
Copy link
Owner

canton7 commented Oct 3, 2011

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
Copy link
Contributor Author

leeovery commented Oct 3, 2011

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
Copy link
Contributor Author

leeovery commented Oct 3, 2011

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

Sorry if Im stating obvious!

@canton7 canton7 closed this as completed Oct 3, 2011
@canton7
Copy link
Owner

canton7 commented Oct 3, 2011

All merged in :)

Sorry for the delay, was busy with something.

canton7 added a commit to fuel-packages/fuel-casset that referenced this issue Oct 19, 2011
* feature/set_group_option_fix:
  Add option prep method so group options are passed through to the add_group method.
  If set_group_option is used with all groups ('*'), then change the default.
  Create the global groups on initialisation, if it doesn't exist.
  set_group_option() will throw an exception if the named group doesn't exist.
  Add new function to tell whether a group exists.

Closes canton7#9
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

No branches or pull requests

2 participants