Skip to content

Integration with the profiler #13

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

Closed
wants to merge 0 commits into from

Conversation

vincentchalamon
Copy link
Contributor

@vincentchalamon vincentchalamon commented Nov 5, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? in progress
Fixed tickets symfony/mercure#1

Menu icon

mercure-menu

Panel

mercure-panel

TODO

@vincentchalamon vincentchalamon force-pushed the master branch 2 times, most recently from 5fdb3ef to a859ec8 Compare November 6, 2019 09:27
@vincentchalamon vincentchalamon changed the title [WIP] Integration with the profiler Integration with the profiler Nov 6, 2019

if ($config['enable_profiler']) {
$container->register('data_collector.mercure', MercureDataCollector::class)
->setArguments([$publishers, $hubUrls])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe that a map hubUrl[publisher] would be better and less error prone? Or the hub URL could be injected in TraceablePublisher's constructor and then retrieved using a getter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to inject the hub Url in the TraceablePublisher because the url is only required by the WebProfiler view on this bundle, not on the lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, we could remove the $hubUrls argument, as it's only used to display the hub url on the profiler. I don't find it so useful cause most of the time the user already know the hub url cause he configured it ^^

Copy link
Member

Choose a reason for hiding this comment

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

Actually it could be useful for other use cases too to be able to access the hub URL. IMHO it fits well in the lib too.

}

if ($config['enable_profiler']) {
$container->register('data_collector.mercure', MercureDataCollector::class)
Copy link
Member

Choose a reason for hiding this comment

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

This definition should be in the XML file and then removed here if $config['enable_profiler'] is false. Doing this allows to have autocompletion in PHPStorm and the like.

Copy link
Contributor Author

@vincentchalamon vincentchalamon Nov 6, 2019

Choose a reason for hiding this comment

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

To do so, I'll need to tag the publisher services and handle them through a CompilerPass to inject them in the MercureDataCollector, and manage the hub url too...

Isn't there an easier way to handle that?

}

if ($config['enable_profiler']) {
$container->register('data_collector.mercure', MercureDataCollector::class)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$container->register('data_collector.mercure', MercureDataCollector::class)
$container->register('mercure.data_collector', MercureDataCollector::class)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

data_collector.<name> was to respect the same naming convention used by symfony/framework-bundle about DataCollectors: https://github.com/symfony/framework-bundle/blob/master/Resources/config/collectors.xml

Copy link
Member

Choose a reason for hiding this comment

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

Yes because it's in FWBundle, but here as we aren't in FWBundle, we shouldn't "pollute" the global namespace and put every services under the mercure. key.

@dunglas dunglas closed this Nov 7, 2019
@dunglas dunglas mentioned this pull request Nov 7, 2019
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Nov 12, 2019
…filer option (vincentchalamon)

This PR was submitted for the master branch but it was squashed and merged into the 4.3 branch instead (closes #12598).

Discussion
----------

Update documentation for MercureBundle: add enable_profiler option

Related feature: symfony/mercure-bundle#13

![panel](https://user-images.githubusercontent.com/995707/68297999-9d299b00-0098-11ea-9662-86062da00bf8.png)

Commits
-------

3705dae Update documentation for MercureBundle: add enable_profiler option
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