Use Set to collect callbacks in OnPageLayout #1583
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When using the OnPageLayout mixin outside the ApplicationController (because one might want to run define those methods only in certain controllers and not in others), AND using an `ActiveSupport::Concern`-style mixin to do that, AND including that mixin in multiple controllers, one ends up with the callback or block called multiple times. Using a `Set` instead of an Array neatly circumvents this issue, as it will only add new unique callbacks. Unfortunately, there is not a good way to test this, as running the `on_page_layout` class method in any controller will expect the `Alchemy::PagesController`to have a callback defined, and the test setup for this spec would make many other specs fail. Ideally, the Set of Callbacks would not be stored in a module instance variable on the `OnPageLayout` module, but on the including controller. That way the functionality (which is extremely useful!) could be scoped a little more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this pull request for?
When using the OnPageLayout mixin outside the ApplicationController
(because one might want to run define those methods only in certain
controllers and not in others), AND using an
ActiveSupport::Concern-style mixin to do that, AND including thatmixin in multiple controllers, one ends up with the callback or block
called multiple times. Using a
Setinstead of an Array neatlycircumvents this issue, as it will only add new unique callbacks.
Unfortunately, there is not a good way to test this, as running the
on_page_layoutclass method in any controller will expect theAlchemy::PagesControllerto have a callback defined, and the test setupfor this spec would make many other specs fail.
Ideally, the Set of Callbacks would not be stored in a module instance
variable on the
OnPageLayoutmodule, but on the including controller.That way the functionality (which is extremely useful!) could be scoped
a little more.