Skip to content

Merge Profiling back to main extension #100

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 1 commit into from
Jul 24, 2016
Merged

Merge Profiling back to main extension #100

merged 1 commit into from
Jul 24, 2016

Conversation

sagikazarmark
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes
Related tickets fixes #95
License MIT

What's in this PR?

Merge back ProfileExtension to the main one and fix the kernel debug fallback problem.

Why?

Separating profiling into a separate class does not make the code cleaner or more readable.

@sagikazarmark sagikazarmark force-pushed the dev-issue95-2 branch 2 times, most recently from 062c37a to 614d55b Compare July 23, 2016 19:57
@sagikazarmark
Copy link
Member Author

@Nyholm @dbu please review before merging this into @Nyholm's branch

@@ -20,6 +20,23 @@
class Configuration implements ConfigurationInterface
{
/**
* Whether to use the debug mode.
*
* @see https://github.com/doctrine/DoctrineBundle/blob/master/DependencyInjection/Configuration.php#L31-L41
Copy link
Member

Choose a reason for hiding this comment

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

If you want to have a reference to a line you should probably do it to a tag instead of master:
https://github.com/doctrine/DoctrineBundle/blob/v1.5.2/DependencyInjection/Configuration.php#L31-L41

@Nyholm
Copy link
Member

Nyholm commented Jul 24, 2016

Im 👍

I like how you fixed the kernel.debug thing. I just had some minor comments.

Separating profiling into a separate class does not make the code
cleaner or more readable.

In this commit:

- Profiling moved back to main extension
- Toolbar (profiling) defaults to kernel.debug
@sagikazarmark
Copy link
Member Author

Thanks for the review. Merge whenever you think it's ready. I will merge the other PR then.

@Nyholm Nyholm merged commit bf2a5a4 into dev-issue95 Jul 24, 2016
@Nyholm
Copy link
Member

Nyholm commented Jul 24, 2016

I would like to do a final review before merging to master

@sagikazarmark sagikazarmark added this to the v1.2.3 milestone Jul 24, 2016
@sagikazarmark sagikazarmark deleted the dev-issue95-2 branch July 24, 2016 10:13
@sagikazarmark sagikazarmark modified the milestones: v1.2.3, v1.3.0 Jul 25, 2016
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.

2 participants