Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added flag parameter for enabling AuditController #161

Merged

Conversation

p365labs
Copy link
Contributor

@p365labs p365labs commented Jan 7, 2020

@DamienHarper & @maxhelias Hi all, I implemented a solution to programmatically disable AuditController::class,
this way a new configuration parameter could be set to false and that controller and its routes won't be available.

The use case to cover is a use case in which the library is used to store informatin on the persistence layer but they will be available in a different way to the user, for example through API.
The controller routes won't be exposed to any environment.

The parameter is:

  • enable_audit_controller

and has been added to the Configuration.php file.

  • default value is true

reference issue #153

@p365labs p365labs force-pushed the feature_flag_audit_controller branch from 100f74d to 5c46317 Compare January 7, 2020 18:15
Copy link
Owner

@DamienHarper DamienHarper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p365labs thanks for the contribution.
Though, your PR does not do exactly what you expect it to do: the compiler pass removes the controller definition but routes still get registered and trying to access such a route triggers a LogicException with message "DH\DoctrineAuditBundle\Controller\AuditController" has no container set, did you forget to define it as a service subscriber?

@p365labs p365labs force-pushed the feature_flag_audit_controller branch from 481fc95 to 37836ff Compare January 12, 2020 09:12
@p365labs
Copy link
Contributor Author

Hi, @DamienHarper you're right, I missed a piece.
Right now I've implemented a custom route loader which based on parameter flag,
loads the annotations from the controller.

Copy link
Contributor

@maxhelias maxhelias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested yet but it looks very promising

src/DoctrineAuditBundle/Resources/config/services.yaml Outdated Show resolved Hide resolved
src/DoctrineAuditBundle/Routing/CustomLoader.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
Copy link
Contributor

@maxhelias maxhelias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested yet but that seems good to me

composer.json Outdated Show resolved Hide resolved
src/DoctrineAuditBundle/AuditConfiguration.php Outdated Show resolved Hide resolved
src/DoctrineAuditBundle/AuditConfiguration.php Outdated Show resolved Hide resolved
src/DoctrineAuditBundle/AuditConfiguration.php Outdated Show resolved Hide resolved
src/DoctrineAuditBundle/AuditConfiguration.php Outdated Show resolved Hide resolved
src/DoctrineAuditBundle/Routing/CustomLoader.php Outdated Show resolved Hide resolved
Copy link
Owner

@DamienHarper DamienHarper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks for the contribution!

@DamienHarper
Copy link
Owner

@p365labs a conflict remains about composer.json. Please, fix it so that I can merge ;)

@p365labs p365labs force-pushed the feature_flag_audit_controller branch from deee012 to 12c9017 Compare January 15, 2020 22:33
@p365labs
Copy link
Contributor Author

@DamienHarper my fault, I probably forgot to rebase from master. now should be ok.

@DamienHarper DamienHarper merged commit 309dd57 into DamienHarper:master Jan 15, 2020
@p365labs p365labs deleted the feature_flag_audit_controller branch January 16, 2020 07:06
@DamienHarper DamienHarper added this to the 3.3.0 milestone Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants