Skip to content
This repository was archived by the owner on Nov 30, 2022. It is now read-only.

Non-Embedded App Support - #935

Draft
xplodedthemes wants to merge 5 commits into
gnikyt:masterfrom
xplodedthemes:non-embedded
Draft

Non-Embedded App Support#935
xplodedthemes wants to merge 5 commits into
gnikyt:masterfrom
xplodedthemes:non-embedded

Conversation

@xplodedthemes

@xplodedthemes xplodedthemes commented Aug 7, 2021

Copy link
Copy Markdown

Bring back ShopSession service and Shopify Auth from v16
1 Middleware alias "verify.shopify" that will now points to 2 different Middlewares (VerifyShopifyExternal or VerifyShopifyEmbedded) depending on the "appbridge_enabled" config.
@xplodedthemes xplodedthemes changed the title Non embedded Non-Embedded App Support Aug 7, 2021
@gnikyt

gnikyt commented Aug 9, 2021

Copy link
Copy Markdown
Owner

Thanks for this.

Things I see that I will attempt to work on...

  • AuthorizeShop... is removed in v17, we can somehow massage this into whats existing on v17
  • Removal of CookieHelper... dont think its needed anymore
  • Removal of ShopSession... v17 uses ShopContext injected into the model, somehow massage this in

@gnikyt
gnikyt marked this pull request as draft August 9, 2021 13:20
@gnikyt gnikyt added the feature Enhancement to the code label Aug 9, 2021
@gnikyt

gnikyt commented Aug 9, 2021

Copy link
Copy Markdown
Owner

@xplodedthemes Is there anything else missing, just curious on a couple things as its been a while since I touched non-embedded.

  1. Is there a setting to toggle embedded and non-embedded? Or are you simply swapping the middleware
  2. More with point 1... is there Blade layout modifications to not load appbridge?
  3. Whats the current flow, are you still pointing to /authenticate

@aveeto

aveeto commented Aug 9, 2021

Copy link
Copy Markdown

@osiset

That's it, I think.

  1. It is all based on the appbridge_enabled option.
  2. Nothing has changed in the views. There was already a condition whether to load app bridge or not within the layout based on the appbridge_enabled option
  3. Yes, I brought back to the authenticate.oauth route from v16.

In v16, there was a login page view that I am currently still using. We can actually use this view for both Embedded and Non-Embedded apps, and it would work fine. The only thing is that, with the non-embed, it will always re-direct to the login page since the session is not saved. It becomes more of an app install page instead of a login page.

@gnikyt

gnikyt commented Aug 9, 2021

Copy link
Copy Markdown
Owner

I think app_bridge_enabled option is currently removed in master? I'd have to check, if not, then that makes sense and the combined work can begin.

@xplodedthemes

Copy link
Copy Markdown
Author

@osiset No, the option is still there in master: appbridge_enabled. Note that it's appbridge without underscore.

https://github.com/osiset/laravel-shopify/blob/master/src/resources/config/shopify-app.php

@valdisd

valdisd commented Sep 1, 2021

Copy link
Copy Markdown

Any updates here? Would love for this to be merged.

@gnikyt

gnikyt commented Sep 8, 2021

Copy link
Copy Markdown
Owner

@valdisd Not at this time, still trying to get time to work on this.

@valdisd

valdisd commented Sep 8, 2021

Copy link
Copy Markdown

I tested this and when installing this goes into /shopify/authenticate/oauth redirect loop.

Works if the installation is done with app bridge enabled and disabled afterwards. Will try to locate the problem.

@gnikyt

gnikyt commented Sep 8, 2021

Copy link
Copy Markdown
Owner

@valdisd I believe the code provided by OP works or partially works, I just need it reworked into the existing code to not have duplication. I outlined some points previously, but feel free to poke around :)

@apurbajnu

Copy link
Copy Markdown

Any update on this?

@o2b3k

o2b3k commented Oct 4, 2021

Copy link
Copy Markdown

@osiset Hi any update on this! I really need this feature

@o2b3k

o2b3k commented Oct 20, 2021

Copy link
Copy Markdown

😏

@Aryanhasanzadeh

Copy link
Copy Markdown

@osiset
First of all, thank you for the good library you have created
Secondly, please check this new feature soon so that we can continue to use your library in the project

@Aryanhasanzadeh

Copy link
Copy Markdown

@xplodedthemes

I applied your commits manually in the project, but I still have the following error, you can help me ?

impo

@t08-apporio

Copy link
Copy Markdown

hi @xplodedthemes composer installed your fork. it's currently stuck in a redirect loop. The url in question is authenticate/oauth. I updated appbridge_enabled to false and also disabled the embedded app option in app setup. Any idea what the issue could be?

@Aryanhasanzadeh were you able to make any progress?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

feature Enhancement to the code help-wanted Contributor help would be nice!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants