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

Reimplementation of the RedisProfilerStorage #301

Merged
merged 2 commits into from
Dec 11, 2016
Merged

Reimplementation of the RedisProfilerStorage #301

merged 2 commits into from
Dec 11, 2016

Conversation

GijsL
Copy link
Contributor

@GijsL GijsL commented Nov 3, 2016

Because the built-in support for Redis as profiler datastore was dropped as of Symfony3 I reimplemented it in this bundle. It is basically a copy of the original RedisProfilerStorage and adjusted to handle the client configuration from this bundle. I also copied the phpunit test files.

Let me know if I need to do more

@GijsL
Copy link
Contributor Author

GijsL commented Nov 15, 2016

@Seldaek @snc I would like to see this PR merged.
Is this bundle still maintained? Is this feature not wanted? Or is there more work to be done?
Please advice

@snc
Copy link
Owner

snc commented Nov 15, 2016

LGTM... do you know the reason why this was dropped in Symfony3?

@GijsL
Copy link
Contributor Author

GijsL commented Nov 16, 2016

No idea, and cant find any discussion on why it was removed.
Maybe the dependency on a php extension.

@Pouix
Copy link
Contributor

Pouix commented Nov 17, 2016

In Symfony versions prior to 3.0, profiles could be stored in files, databases, services like Redis and Memcache, etc. Starting from Symfony 3.0, the only storage mechanism with built-in support is the filesystem.

You can also create your own profile storage service implementing the ProfilerStorageInterface and overriding the profiler.storage service.

Here's what you can read in the doc since 3.0 version.

@GijsL
Copy link
Contributor Author

GijsL commented Nov 17, 2016

That documentation does not mention why it was removed. I reimplemented it in the way that was suggested, and as a part of this bundle because we already use it in our projects.

Would be great if this PR could be merged!

@syrm
Copy link

syrm commented Nov 28, 2016

Interested too

@xavierleune
Copy link

Yes, in fact support was dropped "because" without further discussion.
It seems it was a matter of simplicity and I regret that the community has not been involved in this choice.

@GijsL
Copy link
Contributor Author

GijsL commented Dec 7, 2016

@snc @Seldaek What are your thoughts on this PR? I believe it adds enough value to be merged, but please tell me if you have other thoughts. I need a stable and maintainable solution and this bundle seems the right place.
I prefer not to maintain my own version of this bundle :)

@snc snc merged commit 00f0261 into snc:master Dec 11, 2016
@snc
Copy link
Owner

snc commented Dec 11, 2016

Merged.

@syrm
Copy link

syrm commented Dec 11, 2016

Nice !

@GijsL GijsL deleted the feature/profiler-storage branch December 11, 2016 19:21
@GijsL
Copy link
Contributor Author

GijsL commented Dec 12, 2016

Thanks!
Im not sure what the procedure is for this, but could you release a new version?

@GijsL
Copy link
Contributor Author

GijsL commented Jan 20, 2017

@snc Could you release a new version please?

@GijsL
Copy link
Contributor Author

GijsL commented Feb 28, 2017

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.

5 participants