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

Plugin API Refactor: Filter and Theme Helpers #931

Closed
wants to merge 1 commit into from

Conversation

jgable
Copy link
Contributor

@jgable jgable commented Sep 28, 2013

  • Refactor doFilter to allow returning a promise from a filter handler
    and to also return a promise itself
  • Move the logic out of the registerThemeHelper calls and into their own methods so
    we could test them in isolation.
  • Pass the server to the initPlugins method and move the call from
    ghost.init() to server startup.

This should cover the first 3 checkboxes in #769 for the plugin architecture.

return plugins.init(this, pluginsToLoad).then(function (loadedPlugins) {
if (!_.isArray(pluginsToLoad)) {
// Load the default plugins from settings if not passed
return models.Settings.read("activePlugins").then(function (activePluginsSetting) {

This comment was marked as abuse.

This comment was marked as abuse.

jgable referenced this pull request in ErisDS/Ghost Sep 29, 2013
issue TryGhost#932

- handles outputting assets for the admin and theme which have a version query string
@ErisDS ErisDS mentioned this pull request Sep 29, 2013
@jgable
Copy link
Contributor Author

jgable commented Oct 1, 2013

Updated

Changed activePlugins default to JSON.parse(instance.settings('activePlugins'))

}

// Recursively call ourself again with the active plugins
return self.initPlugins(server, settingValue);

This comment was marked as abuse.

This comment was marked as abuse.

@frysbe
Copy link

frysbe commented Oct 25, 2013

@jgable: great work on this PR, BTW. I hope it gets merged, because I am working on an async plugin.

I have 1 other small point: I wonder if initPlugins is the right place to set ghost.server. It seems like the code in initPlugins can be used to initialize newly activated plugins while the server is running, so it might run more than once. I can understand that plugins might need ghost.server, but perhaps it can be set directly in server.js?

@jgable jgable mentioned this pull request Oct 28, 2013
- Refactor doFilter to allow returning a promise from a filter handler
  and to also return a promise itself
- Move the logic out of the registerThemeHelper calls and into their own methods so
  we could test them in isolation.
- Assign the server to the ghost instance so the initPlugins method can
  get access to it.
@jgable
Copy link
Contributor Author

jgable commented Oct 29, 2013

Updated

Rebased on latest from master. Included adding some new helpers and refactoring a couple tests to match the new style.

Also, refactored the initPlugins per @frysbe suggestions. Specifically, not calling itself recursively and having server.js attach the express instance onto the ghost object itself.

ghost.server = server;

// Initialize plugins then start the server
ghost.initPlugins().then(function () {

This comment was marked as abuse.

ErisDS added a commit that referenced this pull request Oct 29, 2013
@ErisDS
Copy link
Member

ErisDS commented Oct 29, 2013

Manually merged along with error handling improvements.

@ErisDS ErisDS closed this Oct 29, 2013
@frysbe
Copy link

frysbe commented Nov 13, 2013

@jgable Thanks again for your work! I'm happy this has been merged.

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.

3 participants