Make the downloads controller more flexible for own sources

[Toflar]

With the introduction of the new DownloadsController, I could actually turn my own 600 lines of code in my custom downloads content element into just 20. Everything works the same: I need previews, download handling, sorting etc. The only difference is: I have a different source folder in the settings and additional $filesystemItems->filter() calls.

This PR introduces an AbstractDownloadContentElementController which greatly enhances reusability.

[fritzmg]

Shouldn't we apply this to all content elements and front end modules then?

[Toflar]

What? No - only what qualifies as a use case.

[m-vo]

I also think it's a good idea to treat the controllers as if they were final classes, i.e. that they are not meant to be reused. But I can understand the use case here.

Some other options, we should at least consider to communicate this idea:

1. Extract the functionality in a helper class/service, like the already existing FileDownloadHelper, so that the controller itself gets 'thinner' and does not need to be reused. (Is this feasible in this scenario?)

2. Also move the class to abstract class AbstractDownloadsController and add an empty default implementation final DownloadsController extends AbstractDownloadsController, to make it clear that the abstract is intended to be reused.

3. Alternatively mark all other controllers with @final and make them final in Contao 6.

Wdyt? I'm also fine with the simple changes as they are in this PR. ☺️

[Toflar]

I actually think the logic is controller-specific so it's good to reside where it does. But I like the idea of extracting it into an abstract. Wdyt, @contao/developers ?

[leofeyer]

extracting it into an abstract

Yes. 👍

(I‘d also be fine with a helper service as suggested by @m-vo above.)

[Toflar]

I'll work on it for 5.3 then :)

[leofeyer]

The PR is still based on the 5.2 branch, therefore I request changes so we don‘t accidentally merge it before it has been rebased.

[Toflar]

This is ready for review now. I have added a few review comments myself to explain my design thoughts.

I have tested the changes in the demo and everything still works.

[m-vo]

This looks very promising to me!

I was wondering if we could also move $this->handleDownload() into the abstract class? We could do this by overwriting __invoke() to handle the download and then call parent::__invoke(). Basically, so that all open todos can be solved in the future without anyone needing to update their implementations.

[Toflar]

$this->handleDownload() is in the abstract class for exactly that reason?

[m-vo]

$this->handleDownload() is in the abstract class for exactly that reason?

Yes, but I would also put the call to it in the abstract class as well. Because now you still need to add it to your implementation of getResponse() and remove it later on when we switch to a route that handles it.

Can we do something like this?

public function __invoke(…): Response {
    $this->handleDownload();
    parent::__invoke(…);
}

[Toflar]

Ah, now I understand. Yes of course, that makes sense! Done.

[m-vo]

I like how much cleaner the downloads controller is looking now. Probably even helps people to better understand the logic. And all the workarounds are moved out of the class as well. 👍

No longer valid

[leofeyer]

Thank you @Toflar.