-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Plugin system #1407
Plugin system #1407
Conversation
Conflicts: request.js
I like the concept, this is definitely the direction we want to head IMO. I'm a little worried about unexpectedly breaking things though, like #1409. I have been swamped lately (just accepted a job with Automattic!) so I'll need at least a few days to do a proper review. @mikeal @FredKSchott @seanstrom etc. any help appreciated. |
I'm really not changing that much at this stage, just moving code around, the instance bug with auth is very silly, but apparently not caught by any of the tests. |
I agree, I feel like I could have caught that in reviewing though, so I'd like to be more thorough this time. |
I don't have time to do a full review but I will say that I like this approach. I'll be impressed if you can actually work the forwarding logic in to a plugin, that logic always seemed to infect parts of the code I never thought I'd have to worry about :) |
@mikeal I'm not familiar with the entire code base in details yet tbh, but still the refactoring should start from somewhere. |
@nylen off topic, but congrats on the new job!! :) |
I'll be doing a thorough review today :) |
Thanks @seanstrom take a look at this PR #1406 too, as most of my latest ideas are there. Also not really that much changed at this point, I'm really just moving code into separate scopes as I see fit. For example the last one - the redirect one is just something that I thought it deserves it's own scope, and in fact except two of the fields in this newly created class are used externally, so it looks like a separate entity after all. |
Closes #1400
Closes #1406
I just merged #1400 and #1406 here + refactored all of the 4 plugins to use similar structure.