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

Move PHP settings handling to separate class(es) #2

Closed
mrclay opened this issue Mar 16, 2012 · 2 comments
Closed

Move PHP settings handling to separate class(es) #2

mrclay opened this issue Mar 16, 2012 · 2 comments

Comments

@mrclay
Copy link

mrclay commented Mar 16, 2012

A string alteration component should not alter PHP settings--especially those put in place to protect a system from PHP overusing resources--but I realize the pragmatism in CSSmin choosing to do this by default.

I think this functionality should probably be moved to a component dedicated to sniffing and altering settings like these. I think at most CSSmin should keep the getSuggestedPhpLimits() method, or have those in a PHPDoc annotation that could be read through reflection.

Should run() return the settings to their original values before returning? Maybe the settings shouldn't be altered until run() is called... anyway, something to think about.

@tubalmartin
Copy link
Owner

I agree.
I've committed some changes right now although tomorrow I plan to further enhance the PHP settings since right now I'm forcing to set a specific amount of resources that in some environments may be even higher, so I'll add a check on that.

@tubalmartin
Copy link
Owner

PHP settings handling done:

  • Added max_execution_time setting
  • Users can override any setting (four public methods implemented)
  • Always prefer higher settings
  • Allow -1 for "no memory limit"
  • PHP settings are applied on each run() call

I agree PHP settings should be in a separate class but since this is a standalone class (my company uses it in different projects) we prefer to leave it as is although you may adapt (fork) the code to your needs for Minify ;)

Thanks for your feedback Steve!

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