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

Modules not isolated, rely on foreknowledge of includer #371

Closed
atz opened this issue Sep 1, 2016 · 4 comments
Closed

Modules not isolated, rely on foreknowledge of includer #371

atz opened this issue Sep 1, 2016 · 4 comments
Labels

Comments

@atz
Copy link
Contributor

atz commented Sep 1, 2016

How does a module with no ancestors call super in a method? (Multiple methods.)

Example:
https://github.com/projecthydra/hydra-head/blob/master/hydra-access-controls/lib/hydra/policy_aware_access_controls_enforcement.rb#L9

I know this recurs generally in the Hydra stack but it is bad design. The module doesn't check at include time whether its needs are met and the includer shouldn't be forced to debug runtime errors or monkey-patch the module to inject its dependencies (in this case blacklight access controls enforcement).

This is not the composition pattern that was intended.

@jcoyne
Copy link
Member

jcoyne commented Sep 1, 2016

This is a common pattern in Rails: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/callbacks.rb#L292 And it was my impression that those programmers knew more than I did, so I copied them. I agree it's not the best, but I don't have the inclination or time to refactor it. Patches welcome!

@atz
Copy link
Contributor Author

atz commented Sep 1, 2016

@jcoyne, that isn't comparable. ActiveRecord::Callbacks accounts for its dependencies here:
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/callbacks.rb#L268-L277

    module ClassMethods # :nodoc:
      include ActiveModel::Callbacks
    end

    included do
      include ActiveModel::Validations::Callbacks

      define_model_callbacks :initialize, :find, :touch, only: :after
      define_model_callbacks :save, :create, :update, :destroy
    end

Now that is obfuscated and heavy on meta-programming, but isn't unmodular. But by the time it calls _run_touch_callbacks, it itself has done:

define_model_callbacks :initialize, :find, :touch

Given the context, we can tell that ends up here:
https://github.com/rails/rails/blob/master/activemodel/lib/active_model/callbacks.rb#L103

It does not rely on the includer having already defined _run_touch_callbacks. So either you misunderstand what I'm describing, or you need a more serviceable example. I don't think what I'm indicating here is a common pattern in rails, or even an uncommon one: I expect it to appear zero times. If it does occur, I expect the maintainers would regard it as a mistake. Which is what I expect here.

@atz atz added the Bug label Jan 18, 2017
@atz
Copy link
Contributor Author

atz commented May 22, 2017

@jcoyne Have I described it clearly enough? Do you agree this is not just a "nice to have", but actually a flaw in design?

@atz
Copy link
Contributor Author

atz commented May 31, 2017

See also: projectblacklight/blacklight#1588

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants