KnpLabs / KnpRadBundle

Rapid Application Development for Symfony2 [UNMAINTAINED]
MIT License
291 stars 49 forks source link

createAccessDeniedException() must be public #189

Closed gaetan-petit closed 10 years ago

gaetan-petit commented 10 years ago

I'm installing a project with KnpRadBundle.

I'm not able to make it run unless the visibilty of this method in /vendor/knplabs/rad-bundle/Knp/RadBundle/Controller/Controller.php on line 11 is set to public.

docteurklein commented 10 years ago

Unfortunalty, symfony base controller (which we extend, which is bad) sets the visibility of this method to public (which is totally strange IMHO, because no one should call this method from the outside).

This statment is true for all other helper methods (generateUrl, renderResponse, ...) Anyway, unless symfony makes changes in their own base controller (which I doubt), we will have to do something.

the easier is to not override this method in our base class (simply remove it), or if we keep it, make it public, as you proposed). It's a quick fix and if you want to provide a PR, please do :) would be awesome.

Otherwise, the other cleaner solution is to not use a base controller, but instead compose your own class and inject only the helpers you want.

There is one for rendering responses, another one for routing, another one for security, ... and you can make your owns.

example:


services:

    app.controller.inscription:
        class: App\Controller\InscriptionController
        arguments:
            - '@form.factory'
            - '@knp_rad.controller.helper.doctrine'
            - '@knp_rad.controller.helper.session'
            - '@knp_rad.controller.helper.response'

this would be the best approach (to my mind). It's called "controller as a service".

Anyway, it doesn't resolve the inheritance problem raised here, we have to fix this visiblity problem.

Thanks for pointing this out!

gaetan-petit commented 10 years ago

@docteurklein : thanks for the quick answer, I can prodive a pull request. Could you point me the correct branch? I could not find the method in the controller for the main & develop branches.

docteurklein commented 10 years ago

ho, I must have talked too quickly. This problem has been solved in https://github.com/KnpLabs/KnpRadBundle/commit/88f23630053c894d6e65faad98d4b92b8c271310

Upgrading to v2.6 , v2.5.2, v2.5.1 or v2.5.0 should resolve the problem. What version of knpradbundle are you running ?

gaetan-petit commented 10 years ago

v2.3.2 (I see this is the recommended version on KNP lab site), the project was created by KNP lab too. Is there any compatibility risk with updating to newer version? Maybe it's time to update the website's version too.

docteurklein commented 10 years ago

this project trying to follow semver, you shouldn't have crazy BC breaks.

Here is the full changeset https://github.com/KnpLabs/KnpRadBundle/compare/v2.3.2...v2.6 We dropped support for 5.3 (which does not means it doesn't work on 5.3, just that we don't verify it anymore).

The only BC break I see (to be verified) is https://github.com/KnpLabs/KnpRadBundle/commit/fe64dfcbc58c5bba5d69cc26f7bf767bd9058a0f, but it should not be very often used.

gaetan-petit commented 10 years ago

Thanks you @docteurklein