Skip to content

Added a configuration option to define default driver for Image module #9

Closed
wants to merge 1 commit into from

3 participants

@alrusdi
alrusdi commented Dec 14, 2012

Reasons is in this Feature request http://dev.kohanaframework.org/issues/4677

@acoulton

@enov I'm not sure about this, what do you think? I think it's a valid usecase (I ran into the same issue recently) but not sure if we can take this for 3.3 - as there will be existing code that has assigned Image::$default_driver and that value should take precedence over our default config.

Maybe if we tweaked @alrusdi's implementation so that there is no default config, so end-users can provide one if they want but otherwise we use Image::$default_driver?

@acoulton

And @alrusdi thanks, and sorry this has taken so long to get looked at.

@enov enov referenced this pull request in kohana/orm Oct 10, 2014
Merged

Add standalone build for 3.4 series #106

@enov
Kohana PHP Framework member
enov commented Oct 17, 2014

There are currently three ways to change the default GD driver:

  • Bootstrap, as suggested by @Zeelot in the Redmine issue linked above
  • Extending the Kohana_Image class through the CFS, tweeking factory, as suggested by @alrusdi in Redmine
  • Also extending the Kohana_Image class through the CFS, but just public static $default_driver = 'Imagick';

@Zeelot is rightfully arguing that there is no need to invoke a Config call if it can happen through bootstrap. But @alrusdi is arguing that it is less intuitive. Also adding Config to the module will add an additional dependency to kohana/core besides the Kohana_Exception and Kohana::$log that it uses.

Acknowledging all this, I am leaning towards @alrusdi's argument, that config is a better place for a default value. Regarding performance, invoking a Config call every time is Image is used might not have any penalty, taking into consideration the already CPU intensive calls that Image operation does through GD or Imagick.

Maybe if we tweaked @alrusdi's implementation so that there is no default config, so end-users can provide one if they want but otherwise we use Image::$default_driver?

A NULL value for the 'default_driver' key? That might be needed if we want this for 3.3/develop. So we can still use the old ways of changing a default driver.

@acoulton

@enov I agree that config is a better place for driver configuration - for consistency with all other modules, if nothing else. Obviously on a blank sheet you'd probably pull all of that external to the module into a service container or similar, but hey ho.

A NULL value for the 'default_driver' key?

Exactly. So if you drop this into an existing project then Image::$default_driver wins unless you then update your app to provide default_driver in config instead.

@acoulton

As it looks like @alrusdi has removed his fork (fair enough since it was so old) I have updated this in #20 and will close this. @alrusdi if you're still there, thanks for the contribution.

@acoulton acoulton closed this Oct 20, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.