Protected functions, could be public?

[camfindlay]

Use case here is if I want to use an Extension to add additional controller actions I currently cannot call the getDocumentFromID function in my new controller action due to protected method visibility.

Could getDocumentFromID and getDocumentIdFromSlug be changed to public to allow use from other classes otherwise will have to duplicate the methods.

See https://github.com/silverstripe/silverstripe-dms/blob/2922e83dc14773b1354f25d48d97ccffcc8c8621/code/model/DMSDocument_Controller.php#L29

[robbieaverill]

Why not! Want to PR?

[dhensby]

This change would be a breaking API change because anyone extending the class and overriding that method with their own protected function will get an error from PHP because you can't make a public method protected in descendant classes :/

[camfindlay]

@dhensby fair play. In this case what is a workable way forward? Copy logic into my extension/subcontroller or declare a major release change on the module to allow for better extension of the controller? (or could always contribute the feature we're working on, storing html versions of pdf docs with the DMSDocument object and rendering on a controller action to improve accessibility).

[dhensby]

What I think might work for you is to extend the controller in your project. Open the visibility to public for those methods (public function getDocumentFromID($request) { return parent::getDocumentFromID($request); }).

Then you can set up the URLs to point to that controller (or use injector to replace the controller with yous) then add your extension to your controller instead of the core one...

Does that make sense?

Then we can open the visibility in master or whatever the breaking branch is

[camfindlay]

Cheer @dhensby we've ended up subclassing as suggested, works a treat.

We may look to contribute the new feature we're working on back into the module (or a submodule if not deemed something that fit within the core DMS). We'll raise a new issue to outline the feature ;)

I'll deal with the visibility in the contribution... figure if we're doing a breaking change release we should at least add a nice carrot for users