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

subclass from ActionController::Base not Application controller #434

Merged
merged 1 commit into from
Jul 19, 2017

Conversation

kevinhughes27
Copy link
Contributor

I'm working on mounting ShopifyApp in an application where most of the code is not part of the Shopify application. One of things I came across is that our ApplicationController defines a few before_actions which interfere with the engine. I think this change makes sense since we wouldn't want ApplicationController to interfere with anything inside the engine.

review
@Hammadk @alanhill

@EiNSTeiN-
Copy link
Contributor

How can you be sure no existing app rely on this to add before_actions to their app? If any app do rely on the current behaviour, how do they keep the existing behaviour?

@kevinhughes27
Copy link
Contributor Author

Thats a good point - there isn't really a good way to know if anyone is using this. Intuitively I doubt many people are since it would be a pretty complicated setup.

In general if you need additions to your controllers you should subclass ShopifyApp::AuthenticatedController in your application and add things there. If you really need to add overrides to the controllers that are part of ShopifyApp you can override them entirely.

@kevinhughes27
Copy link
Contributor Author

We could do a bigger version bump to make people more aware of this change.

@EiNSTeiN-
Copy link
Contributor

Can you at least try this branch out on a couple of Shopify channels/apps? It should quickly be obvious if something break.

@kevinhughes27
Copy link
Contributor Author

@EiNSTeiN- yes we should definitely do that

Copy link
Contributor

@alanhill alanhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, as long as everything checks out with #434 (comment) 👍

@@ -1,5 +1,5 @@
module ShopifyApp
class SessionsController < ApplicationController
class SessionsController < ActionController::Base
include ShopifyApp::SessionsConcern
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to include include ShopifyApp::LoginProtection here no?

Copy link
Member

@Hammadk Hammadk Jul 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, never mind. I just remembered that LoginProtection is included in SessionsConcern

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I get tripped up by that too. Every time I always wonder if anyone is actually using this concern in their own controller or if we could simplify it and move this code into the controller.

@kevinhughes27
Copy link
Contributor Author

@swalkinshaw this one too!

Copy link
Contributor

@swalkinshaw swalkinshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good 👍

@kevinhughes27 kevinhughes27 force-pushed the controller-parent-class branch from bcd6fbe to f3bcad1 Compare July 19, 2017 14:05
@kevinhughes27 kevinhughes27 changed the base branch from master to v8.0.0 July 19, 2017 14:05
@kevinhughes27
Copy link
Contributor Author

Merging into v8.0.0 branch. We are going to do a full QA after we're done with all the changes for the next major version.

@kevinhughes27 kevinhughes27 merged commit 15a264d into v8.0.0 Jul 19, 2017
@kevinhughes27 kevinhughes27 deleted the controller-parent-class branch July 19, 2017 14:06
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.

5 participants