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

Database to fire events #105

Closed
5 tasks
lonnieezell opened this issue Jun 6, 2016 · 6 comments
Closed
5 tasks

Database to fire events #105

lonnieezell opened this issue Jun 6, 2016 · 6 comments
Labels
enhancement PRs that improve existing functionalities
Milestone

Comments

@lonnieezell
Copy link
Member

lonnieezell commented Jun 6, 2016

I've been thinking about how the toolbar collectors work lately, and think it might make more sense to have some of the modified to fire events instead of storing data that is accessed by the Collectors. This is cleaner code, and allows for a lot more flexibility. The database engine is the first one that really makes to me. By having events fired off listeners could be provided that:

  • collect data to be shown in the Debug Toolbar
  • analyze queries to automatically detect slow queries and tell the user
  • analyze queries (in MySQL) using EXPLAIN to detect queries that could benefit from rewriting or missing indexes.
  • etc.

This would require refactoring the Database class to fire an event, and create a new listener for collecting the query information.

Enhancement Checklist:

  • Component(s) modified
  • Unit testing updated
  • User guide updated
  • Securely signed commits
  • Conforms to style guide
@lonnieezell lonnieezell added the enhancement PRs that improve existing functionalities label Jun 6, 2016
@lonnieezell lonnieezell added this to the Pre-Alpha 2 milestone Jun 6, 2016
@jim-parry
Copy link
Contributor

Makes sense, especially the query analysis with explain ... that has been a can of worms in CI3.

@prezire
Copy link
Contributor

prezire commented Nov 27, 2016

Instead of refactoring most of the Database class, why not use Traits instead? Laravel seems to have no trouble putting a lot of new features via Traits.

@prezire
Copy link
Contributor

prezire commented Nov 27, 2016

@lonnieezell
Copy link
Member Author

Hooks in CI have been changed quite a bit since v3 so it's good to go. Though I've been considering modifying to use an extra class as the "package" like Laravel does, but only because the class forms a contract so all listeners know exactly what's there. Haven't decided if that's happening or not, yet.

As for refactoring - it's a tiny refactor that's needed. Instead of saving the query you broadcast and event with the query as the payload. That's all that would have to change there. The Toolbar's Database portion would need to be modified, but that's no biggie. Part of me thinks all toolbar collectors should have event listeners to grab their info from, but I'm undecided on that part, also, if just for the performance hit, but that would likely be offset by memory and storage considerations that wouldn't be necessary in production.

The flexibility of having these converted to events, though, instead of saving the debug info, could be huge. People could create all sorts of things that work with them, form a simple "slow log" collector that is db-agnostic, to simply displaying all queries in STDOUT so you could watch, or any number of other things I haven't really thought of yet. Seems very promising.

As for Taylor's love of traits - I personally think he uses too many and could probably allow a number of them to be composed into the class as class instances instead of traits and be better off. But that wouldn't provide the magical use-case he's so fond of. The way I've been taught and seen for myself a bit, is that an over-reliance of traits is bad, if only in part because of the way inheritance and method overriding works when you start combining traits and inherited classes, etc. Traits have their uses, and I believe we have one or two currently, but I don't personally believe they're the magic bullet that Taylor does.

@InsiteFX
Copy link
Contributor

This is what I said at the start of CI 4 that an event system should be built into it even the ci system could use it., I have to agree with you on over doing traits etc;

@prezire
Copy link
Contributor

prezire commented Nov 28, 2016

I'm not sure if that's all of it, but there are actually a few Traits in laravel https://laravel.com/api/5.3/traits.html. and yes, i do love events too!

lonnieezell added a commit that referenced this issue Dec 3, 2016
…f saving all queries. Removed getQueries method from db. First step toward #105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Development

No branches or pull requests

4 participants