Skip to content

[FrameworkBundle][Workflow] Add a way to enable the AuditTrail Logger #21933

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

Merged
merged 2 commits into from
Mar 10, 2017

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Mar 8, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

This will increase the visibility of the Listener. We could encourage people to use at least %kernel.debug% value.


Note for the merge: There are two commits, this is done on purpose (2 different things, but easier to do only one PR)

@@ -480,13 +479,24 @@ private function registerWorkflowConfiguration(array $workflows, ContainerBuilde
// Add workflow to Registry
if ($workflow['supports']) {
foreach ($workflow['supports'] as $supportedClassName) {
$strategyDefinition = new Definition(ClassInstanceSupportStrategy::class, array($supportedClassName));
$strategyDefinition = new Definition(Workflow\SupportStrategy\ClassInstanceSupportStrategy::class, array($supportedClassName));
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

To reduce the number of use statement and to be consistant with other "...\Workflow" usage.


// Enable the AuditTrail
if ($workflow['audit_trail']['enabled']) {
$listener = new Definition(Workflow\EventListener\AuditTrailListener::class);
Copy link
Member

Choose a reason for hiding this comment

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

use a use statement instead? more "SF style" to me

Copy link
Member Author

@lyrixx lyrixx Mar 9, 2017

Choose a reason for hiding this comment

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

Right now, there are more usage of "this style" than FQCN

Copy link
Member

Choose a reason for hiding this comment

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

Just saw that, in the Workflow namespace in fact. OK then.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

josephdpurcell pushed a commit to josephdpurcell/symfony that referenced this pull request Mar 10, 2017
… the AuditTrail Logger (lyrixx)

This PR was squashed before being merged into the 3.3-dev branch (closes symfony#21933).

Discussion
----------

[FrameworkBundle][Workflow] Add a way to enable the AuditTrail Logger

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

---

This will increase the visibility of the Listener. We could encourage people to use at least  `%kernel.debug%` value.

---

Note for the merge: There are two commits, this is done on purpose (2 different things, but easier to do only one PR)

Commits
-------

633c039 [Workflow] Added the workflow name in log generated by AuditTrailListener
b786bcc [FrameworkBundle][Workflow] Add a way to enable the AuditTrail Logger
@lyrixx lyrixx merged commit 633c039 into symfony:master Mar 10, 2017
@lyrixx lyrixx deleted the workflow-audit-trail branch March 10, 2017 09:45
lyrixx added a commit that referenced this pull request Apr 5, 2017
This PR was merged into the 3.3-dev branch.

Discussion
----------

[Workflow] sync the changelog

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20751, #21334, #21933, #21935, #21950
| License       | MIT
| Doc PR        |

Commits
-------

98a18ee [Workflow] sync the changelog
@fabpot fabpot mentioned this pull request May 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants