A couple of questions about Glide image service class

[ghobaty]

Hello 👋,

Background:
In our specific use case we needed to extend Twill's Glide image service \A17\Twill\Services\MediaLibrary\Glide customising and extending its functionality.

While we managed to do what we had in mind, we had a question and a suggestion which we thought could make the class easier to extend, and wanted to run them by you:

1. Is there a reason why $server and $urlBuilder are made private?
2. Would you consider extracting this part to 2 protected methods (e.g. initServer and initUrlBuilder)

   twill/src/Services/MediaLibrary/Glide.php

   Lines 55 to 72 in e85ff14

   	$baseUrl = join([
   	rtrim($this->config->get('twill.glide.base_url'), '/'),
   	ltrim($this->config->get('twill.glide.base_path'), '/'),
   	], '/');
   	$this->server = ServerFactory::create([
   	'response' => new LaravelResponseFactory($this->request),
   	'source' => $this->config->get('twill.glide.source'),
   	'cache' => $this->config->get('twill.glide.cache'),
   	'cache_path_prefix' => $this->config->get('twill.glide.cache_path_prefix'),
   	'base_url' => $baseUrl,
   	'presets' => $this->config->get('twill.glide.presets', [])
   	]);
   	$this->urlBuilder = UrlBuilderFactory::create(
   	$baseUrl,
   	$this->config->get('twill.glide.use_signed_urls') ? $this->config->get('twill.glide.sign_key') : null
   	);

Thanks for your great efforts!

[madalinignisca]

I just stumble over this, and it's not the only class in the project. A lot of classes that you'd like to simple extend from them and override just tiny bits have private private private everywhere.

Suggestion: if you want to block people from overriding, make the Final and state it clear in the documentation you do not support any overriding in the app we build.

[pboivin]

Hi @madalinignisca, I believe in the long term, extensibility is very important, whether via overriding or any other mechanism.

Don't hesitate to share more information on the use cases you have in mind. This can really help shape Twill 3.x and beyond.

A common request is for anything user-related (custom form fields, etc.)

[madalinignisca]

An example is related to the above, in the Glide service. I had to do an alternative to it, copy it, as I could not extend the class.
Anything declared private make is a blocker. Even if you redeclare properties as protected, if you have in the base class a public/protected method that calls one of the private properties, will not do it, as it's bounded to the private base one. Mixing private in classes should not be done at simple developer's choice when this is an open project intended to be used and extended on it.

I would do something particular private only in a closed source package delivered with some supported encription like Zend had in the old days. That being for enforcing something not to be extended. Also I'd set the class to final, to make aware the developer that he can't even extend from it.

I'd suggest a basic non breaking change. Change everything from private to protected. You'd make our lives easier and not end up in a 30% fork over the package in the app.

There are also some classes that would be nice to be allowed to be replaced by configuration, I'll come back to them. Take example spatie/medialibrary how it allows you to change everything.

[pboivin]

Thanks for the feedback @madalinignisca, this makes a lot of sense. Moving everything from private to protected would indeed be an easy change with massive impact.

To your point on configuration, Twill 3.x is definitely moving in this direction! The feature/permissions-management branch introduces configuration for Twill models:
https://github.com/area17/twill/blob/adc5e0b390d9ad871731de5b07709be35d4d2660/config/models.php