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

URGENT UPDATE need to shopify_app v18.1.1 or @shopify/koa-shopify-auth v5.0.3 #1094

Closed
APdevelopments opened this issue Feb 26, 2022 · 35 comments
Closed

URGENT UPDATE need to shopify_app v18.1.1 or @shopify/koa-shopify-auth v5.0.3 #1094

APdevelopments opened this issue Feb 26, 2022 · 35 comments
Labels
bug Bug with the code help-wanted Contributor help would be nice!

Comments

@APdevelopments
Copy link

APdevelopments commented Feb 26, 2022 •
edited
Loading

I'm using Laravel 5.8 and Osiset library 12.1

This morning I got this message:
image

How to solve the problem? Please help, its really urgent
@bougsid
Copy link

bougsid commented Feb 27, 2022

Have you got any idea about this message?

@APdevelopments
Copy link
Author

No, i dont have idea about this message

@shreyansh-shroff
Copy link

We are getting the same message for our apps. Does anyone know the solution?

@mlgrozev
Copy link

We're getting the same message.

@o2b3k
Copy link

o2b3k commented Feb 28, 2022

We're getting the same message. How to solve this issue ?

@paxdigital
Copy link

We're getting the same message.

@joeyfreund
Copy link

Hi there, this is Joey from Shopify.

The source of the problem is in these lines.
It should use App Bridge to do the redirect (the postMessage call you're seeing is the deprecated EASDK call).

I'm not familiar with this project's codebase, but I can see that it's using App Bridge. So, hopefully it won't be too hard to fix this issue.

In the meantime, let me check with our partner managers and ensure that nobody is getting hit by this issue.

@jvillegasmx
Copy link

Getting same message

@joeyfreund
Copy link

@osiset / @lucasmichot , are you actively maintaining this repo?
(I was wondering, since you're the top contributors).

If so, is this something you'd be able to fix?
Also, if you require any help/guidance, please let me know. Thanks 🙏

@nathan-hutchinson
Copy link

Have you guys tried this PR? #1075

@haseebbutt1999
Copy link

@joeyfreund sir, are you getting any solution about this issue?
URGENT UPDATE need to shopify_app v18.1.1 or @shopify/koa-shopify-auth v5.0.3
I am using osiset 16 and laravel 8.
http://joxi.ru/DrlPjWOTK0ZKzr

@mlgrozev
Copy link

mlgrozev commented Mar 1, 2022 •
edited
Loading

@osiset / @lucasmichot , are you actively maintaining this repo? (I was wondering, since you're the top contributors).

If so, is this something you'd be able to fix? Also, if you require any help/guidance, please let me know. Thanks 🙏

@joeyfreund, Do you plan to have Laravel Shopify PHP boilerplate and library in the nearest future?

The PHP community will much appreciate it.

@stevesweets
Copy link
Contributor

Have you guys tried this PR? #1075

trying this now, will report back.

@stevesweets
Copy link
Contributor

@joeyfreund is there a way that i can quickly get a re-check of an app, to see if i've resolved the issue?

@lucasmichot lucasmichot mentioned this issue Mar 1, 2022
Closed
@joeyfreund
Copy link

@mlgrozev , as for your question

Do you plan to have Laravel Shopify PHP boilerplate and library in the nearest future?

This is the Shopify-maintained PHP template - https://github.com/Shopify/shopify-app-php
(It is also what developers get when they scaffold a new PHP app using the Shopify CLI)

@lucasmichot , thank you so much for the clarifications (and honesty).

I was mostly told internally that PHP is more a language for rookies

This is really unfortunate to hear 😞 .
I don't want this to be the focus of the discussion, so all I'm going to say is that I strongly disagree with this statement.

feel free to make PR and we can review it

I'm looking for a PHP dev here who might be able to help with that.

To everybody else in this thread that are currently seeing the warning banner:

  • If your app is built with PHP, the shopify_app and/or @shopify/koa-shopify-auth libraries aren't relevant to your app (they are relevant to Rails and Node apps, respectively)
  • We are still trying to figure out how to best help all partners migrate their apps off of using the (deprecated) EASDK API.
  • The team is working on an update to the text in the banner to make things clearer, it will also provide a link to contact partner support with specific questions (cc @stevesweets ).

@olavoasantos olavoasantos mentioned this issue Mar 1, 2022
Closed
@olavoasantos
Copy link
Contributor

I've opened a PR for this. Could someone help me test it?

Full disclosure, I don't have my PHP environment setup so I couldn't test it properly. When I ran the tests I got this exception which seems unrelated to my work:

1) Osiset\ShopifyApp\Test\Actions\ActivatePlanTest::testRunRecurring
Exception: Missing `laravel/legacy-factories` in composer.json. Please refer to <https://github.com/orchestral/testbench/blob/6.x/README.md#using-legacy-factories>

@mlgrozev
Copy link

mlgrozev commented Mar 1, 2022

Despite endless efforts advocating for taking better care of our open-source communities, I was mostly told internally that PHP is more a language for rookies, and that our partners just have to switch to Ruby and TS.

@lucasmichot This statement is so immature... I personally know many great apps in the ecosystem with PHP codebase, some of which had made great exits.

@osiset is a busy father that has been working very hard for years on this project on his free time with zero support.
When asking for help, he was sent a t-shirt and some stickers
https://opencollective.com/laravel-shopify would have been more welcomed

I'm so happy to hear that, I wish him and his family all the best! He's a great asset to the community and I much appreciate his contribution.

@joeyfreund thanks for clarifying and for having you in this discussion.

@jvillegasmx
Copy link

I have applied the PR from @olavoasantos , the app works normally, but the message it is still there, i think it takes some time to shopify to validated again.
Thanks!

@gnikyt
Copy link
Owner

gnikyt commented Mar 1, 2022

I think I need more info on this. Full page redirect for the auth was always the case and worked. It was originally the way the Ruby gem worked as well. So if thats not the case now, then something obviously needs to be modified. I'll try my best to find out what information I can, but if anyone has links to something helpful that would be good.

Additionally, yes, thank you to everyone for the PRs and supports. I've greatly needed help given my situations with fraud/court things I had to go through. I've been so busy, but I am hoping to finally make time to get back to reviewing the work in the pipeline of the PRs and getting releases out.

@stevesweets
Copy link
Contributor

stevesweets commented Mar 1, 2022 •
edited
Loading

I think I need more info on this. Full page redirect for the auth was always the case and worked. It was originally the way the Ruby gem worked as well. So if thats not the case now, then something obviously needs to be modified. I'll try my best to find out what information I can, but if anyone has links to something helpful that would be good.

Additionally, yes, thank you to everyone for the PRs and supports. I've greatly needed help given my situations with fraud/court things I had to go through. I've been so busy, but I am hoping to finally make time to get back to reviewing the work in the pipeline of the PRs and getting releases out.

I think they just want this to be redirected with App Bridge, pr #1097 I believe will handle this properly. There's a comment on that pr in particular that references how koa now does it which i think is enough to be convinced that it'll work properly.

@osiset Sorry to hear about the personal stuff going on, I really appreciate all your efforts. If I'm able to assist in anyway, I'm absolutely willing to.

@olavoasantos
Copy link
Contributor

Context

For some context, Shopify is deprecating the EASDK support (it's a grandfather library to App Bridge). With that, apps that emit EASDK events are receiving the warning reported in this thread. While most events are emitted through the main library (i.e EASDK), there are some events manually being emitted by 3rd party libraries (e.g. shopify_app gem, @shopify/koa-shopify-auth NPM package and osiset/laravel-shopify). One common case is the Shopify.API.remoteRedirect event that's used to redirect the app. Since the support for EASDK is under deprecation, libraries need to refactor this event to use the App Bridge to handle the redirect.

How does it affect us?

On the src/resources/views/auth/fullpage_redirect.blade.php view, we're currently manually emitting the Shopify.API.remoteRedirect event:

data = JSON.stringify({
  message: 'Shopify.API.remoteRedirect',
  data: { location: redirectUrl },
});

window.parent.postMessage(data, "https://{{ $shopDomain }}");

Because this event is an EASDK event, the Shopify platform detects that an app is emitting the event and flagging them as using deprecated protocols (which results in the warnings seen by developers using laravel-shopify).

How to fix it?

We can fix it by just using the App Bridge library to handle the redirect. This is basically what the PR is doing.

@APdevelopments
Copy link
Author

APdevelopments commented Mar 10, 2022 •
edited
Loading

I implement everything from PR - my app is partial inside iframe and partial need to be full page.

When I want to redirect user to full page I use this link:
<a href="/design" target="_top">New Page Design</a> and target="_top" redirect user to full page

but now I got a problem because it wont redirect user - it show page in iframe inside shopify not an external page

@Kyon147
Copy link
Collaborator

Kyon147 commented Mar 13, 2022

My PR #1075 should solve this issue as it removes the need to do the full page redirect.

If @osiset is super busy as can't review it @lucasmichot can you review the PR for me please as I don't want to be approving my own.

@Kyon147
Copy link
Collaborator

Kyon147 commented Mar 13, 2022

In terms of maintaining, I am around sometimes in the week and try to send some PRs across for the bigger issues. Like the one above.

Anyone else who wants to submit PRs for issues, I'll be happy to look at them and try and get them into the codebase for a next release.

@Kyon147 Kyon147 added bug Bug with the code help-wanted Contributor help would be nice! labels Mar 13, 2022
@APdevelopments
Copy link
Author

APdevelopments commented Mar 13, 2022 •
edited
Loading

Interesting that alert message from Shopify admin panel gone but I didnt update anything...

Also I test the code from PR #1075 but it doesnt work for me - didnt redirect to a new page, use the iframe again

@haseebbutt1999
Copy link

Interesting that alert message from Shopify admin panel gone but I didnt update anything...

Also I test the code from PR #1075 but it doesnt work for me - didnt redirect to a new page, use the iframe again

Update the version of osiset to version 17.1. This is the solution of this issue. Also i tried this with my application, its work for me. Recently my app approved, using package 17.1.

@Kyon147
Copy link
Collaborator

Kyon147 commented Mar 15, 2022

Interesting that alert message from Shopify admin panel gone but I did not update anything...

Also I test the code from PR #1075 but it doesn't work for me - didn't redirect to a new page, use the iframe again

@APdevelopments

The PR #1075 is meant to simplify the process of the auth flow. Basically it avoids the fullpage redirect page which should also stop the auth/token route from running before the OAuth which was one of Shopify's complaints. It also removes one redirect from the whole flow which will speed up the auth ever so slightly.

@APdevelopments
Copy link
Author

APdevelopments commented Mar 15, 2022 via email

@APdevelopments
Copy link
Author

APdevelopments commented Mar 15, 2022 via email

@Kyon147
Copy link
Collaborator

Kyon147 commented Mar 15, 2022

@APdevelopments

This is currently not possible with v17.1 - you can't use this package version for full page apps. You would need to do some work to get it done which Oiset has mentioned in this PR (#935) which was being worked on.

@APdevelopments
Copy link
Author

APdevelopments commented Mar 15, 2022 via email

@stevesweets
Copy link
Contributor

stevesweets commented Mar 15, 2022 •
edited
Loading

Quick check in - I wouldn't like to recommend implementing a non-merged PR, but, I have been successful in ridding myself of this warning by implementing pr #1097

@Kyon147
Copy link
Collaborator

Kyon147 commented Mar 15, 2022

Quick check in - I wouldn't like to recommend implementing a non-merged PR, but, I have been successful in ridding myself of this warning by implementing pr #1097

Interesting, my PR actually removes that route with the old ESDK call. So it would be interesting to see as if merged in you won't ever access the full page redirect as it's technically not needed as you can just do a redirect within laravel to send you to the auth.

@stevesweets
Copy link
Contributor

Quick check in - I wouldn't like to recommend implementing a non-merged PR, but, I have been successful in ridding myself of this warning by implementing pr #1097

Interesting, my PR actually removes that route with the old ESDK call. So it would be interesting to see as if merged in you won't ever access the full page redirect as it's technically not needed as you can just do a redirect within laravel to send you to the auth.

Good question. Ill try to get an answer. I may even be able to trial it.

@Kyon147
Copy link
Collaborator

Kyon147 commented Sep 11, 2022

Released in v17.2.0

@Kyon147 Kyon147 closed this as completed Sep 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Bug with the code help-wanted Contributor help would be nice!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fixed app-bridge redirect on fullpage_redirect auth view olavoasantos/laravel-shopify