Installing a plugin should not overwrite classes or modules in the host application.

[afred]

Rationale: plugins that overwrite classes and modules can undo changes introduced by other gems and plugins, which pose problems of maintenance and compatibility.

Done when guideline is added or rejected, pending discussion.

[afred]

@projecthydra-labs/hydra_plugins_wg 👍 👎 ?

[jcoyne]

What does overwrite mean? Does that mean re-open them to add new methods? Replace existing methods? Or keep them from being autoloaded because they have a higher load precedence?

[jcoyne]

Is this sort of thing permissible?

https://github.com/CanCanCommunity/cancancan/blob/develop/lib/cancan/model_adapters/active_record_adapter.rb#L146-L148

[botimer]

Whether it goes into the first edition of guidelines or not, we should discuss when this kind of override might be valuable and suggest an alternative customization pattern to monkey-patching.

A couple of specific examples would be good -- I don't think we need to outline every architectural problem and solution set in the abstract. I have heard from a bunch of developers, though, that the various extension/customization mechanisms have been hard to wrangle and maintain through upgrades.

To drone on a bit more, let's imagine that someone has a lot of map/GIS content. OpenSeaDragon might be a packaged, default zoom-and-pan thing, but a plugin might supply a Leaflet- or OpenLayers-based alternative. The binding should be "overwritable"/configurable. This doesn't mean that the default component would need to be a plugin, but that it is loosely coupled enough and "selected" in a way that a plugin/config could change it.

[afred]

@jcoyne my thinking was to discourage overwriting classes/modules that will get loaded first, instead of the classes/modules of the same name that are defined upstream.

The goal is to avoid undoing behavior that we want to retain. Here's a ticket in GeoConcerns that addresses this issue: https://github.com/projecthydra-labs/geo_concerns/issues/273

Currently GeoConcerns is using a generator and a template to overwrite FileSetsController in the host app. This poses some maintenance and compatibility problems. It would be better if behaviors added by GeoConcerns were injected via a namespaced module, rather than creating a whole new class that overwrites any other that might be loaded.

[afred]

@jcoyne in the cancan example you cite, it's worth noting 2 things:

1. It is only adding (not modifying) the interface to ActiveRecord::Base. This safer than changing the underlying behavior, but it still opens up compatibility issues, i.e. if somebody else thought it was ok to add their own ActiveRecord::Base.accessible_by from their own engine.
2. The monkey patching here seems to be more for convenience than actual functionality. I think you could do the same thing using a DI approach... i.e.

module CanCan
  def self.accessible_by(model_class, ability, action = :index)
    ability.model_adapter(model_class, action).database_records
  end
end

# and then use it like this
@articles = CanCan.accessible_by(Article, current_ability)

I think adhering to this pattern, rather than monkey patching, would lead to better compatibility and maintainability.

[botimer]

@afred I'll be honest -- I really dislike the "I'll just add a bunch of modules into an existing class" pattern. I would much rather one of two things happen:

1. The designed/expected/acceptable customizations to generic bits (e.g., catalog controller, filesets controller) are broken out as collaborators that perform specific functions, and those collaborators are selected/injected by name/config at the outermost app layer.

This pattern preserves the "ownership" of specific routes, etc., by the mounted engines, delegating specific behavior outward (supplying default implementations). It would decompose and name the behaviors of these central components.

2. The identity of the component to use is indirectly bound (that is, class name in config), so a concrete, specialized instance is used. This would happen by subclassing the generic component and registering it (as, e.g., "the fileset controller").

This pattern takes "ownership" of specific routes, etc., away from the engine and gives it to your application code, which inherits the generic behavior.

[afred]

@botimer I think the 1st option is more stable than the 2nd, but I'm not sure how you envision it working. Could you write up a small example up when you get a chance?

[botimer]

I agree that the first option is slightly more stable, but it does require more work at the core. It is a decomposition/refactoring that requires a lot of thought. The second would just make room for (again, only as an example) an entire controller to be "replaced".

This might happen as simply as something like this:

# Engine config (application.rb)
config.file_sets_controller = 'file_sets'

# Engine routes
resources :file_sets, controller: Rails.configuration.file_sets_controller

# App config override (application.rb)
config.file_sets_controller = 'awesome_sets'

# App controller extension
class AwesomeSetsController < FileSetsController
  # Make Sets Awesome
end

There are two really attractive points to this approach:

1. It becomes very explicit where you are "taking control" (i.e., listed by rake routes, request logs, and in stack traces).
2. Because the routes target your code, rather than classes hidden in gems, it's easier to debug, even if you make no modifications (e.g., def index; log x; super; log y; end;).

[afred]

@botimer It seems like we're getting into guidelines for how plugin authors might allow implementers to customize a plugin. But I think that's more detailed than just having a guideline that says "don't overwrite classes, modules, or methods in the host application".

Are we ok with adding a guideline as written in the ticket title, and create additional guidelines that address how plugins should be customizable?

[botimer]

@afred I think my comments are general suggestions of how behavior can be made customizable in known and stable ways. It's not explicitly about customizing plugins -- it could be, but I was really thinking about core features that afford customization via app code or plugin.

What I'm talking about is marking deliberately generic core code with extension points, as opposed to code that the core developers have considered "final" -- that is, where other components expect it to behave as defined, not with subtle differences based on plugin set.

As an example, the receiver of a "/works" request is a good candidate for this indirection -- the "framework" provides a default controller, and the application provides a specialized controller (by itself or via plugin). The user/app depends on it being customized, and there are no broken expectations because the framework does not depend on "/works" behaving in the strictly generic way. (Any tests that validate the generic behavior would be in the framework by identity, not routes, absent customizations.)

In other words, this is a nod to the benefits of using the open-closed principle in framework elements. Putting something like this in the guidelines would effectively be a request that core code be customized in a consistent way such that plugins can be consistent.

To address the current title, "Installing a plugin should not overwrite classes or modules in the host application": I think there is value in it as a guideline (don't overwite other people's classes), but I also think it is an implicit workaround for lacking a more robust extension mechanism. Specifically, it is suggesting: if you have to modify a class, reopen it and include, because that's the best way to extend now. To echo my initial sentiment, I think that is a brittle and opaque way to extend.

Personally, I would rather make the request for a stronger extension mechanism so plugins can be more well-defined in something we plan for them to do often.

[afred]

@botimer Thanks for that explanation. And I agree that a more codified way to adhere to the open-closed principle that can avoid the pitfalls that come with monkey patching a core class to include new behavior from a module would be great.

Do you think we'd have enough time in this working group to flesh something like that out?

In the meantime, having a very basic guideline (e.g. this ticket) to reinforce the open-closed principle for plugins would not conflict with a more explicit extension pattern that may come along later.

If you have a reason for not adding the basic guideline now, let me know. Otherwise, I'll make a PR to merge it for community review, as was discussed during today's meeting. Thanks!

[botimer]

I'm sorry that I missed the meeting today. I think splitting this into two pieces is fine -- being clear that "don't overwrite classes" is not a blessing of "reopen and include".

We might or might not have sufficient time to think through the alternatives. As with many things, I think it's good to have the digested and draft stuff demarcated -- whether these ideas stay in draft or make it all the way to a recommendation by v1 time is then a less pressing matter, since we are not trying to cover implicit or uncertain ground in the stuff that should be explicit and firm.

Thanks, -Noah

On Tue, Jan 10, 2017 at 2:39 PM, Andrew Myers notifications@github.com wrote:

@botimer https://github.com/botimer Thanks for that explanation. And I agree that a more codified way to adhere to the open-closed principle that can avoid the pitfalls that come with monkey patching a core class to include new behavior from a module would be great.

Do you think we'd have enough time in this working group to flesh something like that out?

In the meantime, having a very basic guideline (e.g. this ticket) to reinforce the open-closed principle for plugins would not conflict with a more explicit extension pattern that may come along later.

If you have a reason for not adding the basic guideline now, let me know. Otherwise, I'll make a PR to merge it for community review, as was discussed during today's meeting. Thanks!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/projecthydra-labs/hydra_plugins_wg/issues/19#issuecomment-271676072, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJtK-Gyag9X33kBAbpjVHjxd-5oO1A-ks5rQ96DgaJpZM4LLCTB .