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

Added read shop from referrer if everything fails #722

Merged
merged 15 commits into from
Mar 11, 2021

Conversation

andthink
Copy link
Contributor

@andthink andthink commented Mar 5, 2021

when using config_api_callback in configuration sometimes $shop is just null if i click on an internal url of my app, but the information is present in the referrer

when using config_api_callback sometimes shop is just null if i click on an internal url of my app, but the information is present in the referrer
@gnikyt
Copy link
Owner

gnikyt commented Mar 8, 2021

Thanks! Given this situation can happen with other cases, maybe you can include support for headers too?

If you look at AuthShopify.php::getData, you'll see code that grabs from input (shop query param), headers (x-shop-domain), and referrer.

Added header support
@andthink
Copy link
Contributor Author

andthink commented Mar 8, 2021

something like this?

 $requestShop = Arr::get(Request::all(), 'shop');
  parse_str(Request::server('HTTP_REFERER'), $refererQueryParams);
  $refererShop = Arr::get($refererQueryParams,'shop');
  $headerShop = Request::header('X-Shop-Domain');

 $shop = $session ? $session->getShop() : $requestShop  ?? $refererShop ?? $headerShop;

I haven't tested it yet, sorry little busy today

@gnikyt
Copy link
Owner

gnikyt commented Mar 8, 2021

The multiple nullcoalescing is a bit hard on the eyes.

My suggestion would be to extract the shop detection into its own private function and have the function return the shop to use.

@andthink
Copy link
Contributor Author

andthink commented Mar 8, 2021

The multiple nullcoalescing is a bit hard on the eyes.

ahahah ok

My suggestion would be to extract the shop detection into its own private function and have the function return the shop to use.

ok I'll look into it

</html>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was added without me noticing, but isn't it needed?

Copy link
Owner

Choose a reason for hiding this comment

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

Thats fine!

@andthink
Copy link
Contributor Author

Ok, I've fixed automatic test errors and added also a parameter to bridge createApp function, without that config_api_callback did not work.
I was working before on version 14 and didn't have this problem, now I've updated and i needed to fix also this

apiKey: '{{ \Osiset\ShopifyApp\getShopifyConfig('api_key', Auth::user()->name ) }}',

@gnikyt
Copy link
Owner

gnikyt commented Mar 10, 2021

Ill review now thanks!

@gnikyt
Copy link
Owner

gnikyt commented Mar 10, 2021

@andthink Looks good to merge. StyleCI noted the file needs some upduates to match Laravel style: here.

Once you update that, I'll pull and add a couple test cases!

@andthink
Copy link
Contributor Author

@osiset Done! I still have cookie/session issues in some cases, but I manage with the ?shop= parameter for now.

@andthink
Copy link
Contributor Author

Found a breaking bug in ShopDomain::fromNative use, please @osiset wait for next UPDATE

@gnikyt
Copy link
Owner

gnikyt commented Mar 11, 2021

Sure :)

@gnikyt gnikyt merged commit 135a400 into gnikyt:master Mar 11, 2021
@darrynten
Copy link
Contributor

this is shockingly dangerous 😱

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

Successfully merging this pull request may close these issues.

3 participants