Allow for custom "shops" table for automatic migrations

[nahid]

laravel-shopify allowing custom auth guard in their latest version. But it not support to allowing migration with custom table for shops in automatic migration.

This PR is allowing developer's to set their own shops table in config to execute migration with given name.

[gnikyt]

Good update, comments inside about a way to clean things up.
Can re-review after.

[nahid]

@osiset, I have already rewritten the code based on your change request. Please review the PR again. Thank you

[gnikyt]

@nahid I am fine at a quick glance except for the fact that the table function is doing more than one thing.

It is named getShopsTable but also the the ability to return a string for a column. I'd split it into two functions... getShopsTable, and shopColumnFor or something?

or... return an array or object for getShopsTable.

Something like:

// Method A
public static function getShopsTable(): array
{
        $shopTable = Util::getShopifyConfig('table_names.shops') ?? 'users';
        $foreignId = Str::singular($shopTable) . '_id';
        return ['table' => $shopTable, 'foreign' => $foreignId];
}

// or .... Method B
public static function getShopsTable(): array
{
        $shopTable = Util::getShopifyConfig('table_names.shops') ?? 'users';
        $foreignId = Str::singular($shopTable) . '_id';
        return [$shopTable, $foreignId];
}

Usage could be something like:

$table = Util::getShopsTable()["table"];

// or ... Method B
list($table) = Util::getShopsTable();
// or ... Method B
$table = Util::getShopsTable()[0];

Or, if that feels messy, then split into two functions.

[nahid]

I think different method for each functionality will added more readability and developer experience as well.
Here is the example:

Util::getShopsTabel(): string

Util::getShopsTableForeignKey(): string

[gnikyt]

@nahid I agree. Go ahead with it :)

[nahid]

@osiset, I have pushed the following changes, Please review again

[gnikyt]

Solid. I'll approve letting Actions run and we'll see what happens. Probably will need test added to Until tests to ensure coverage is there.

[nahid]

Solid. I'll approve letting Actions run and we'll see what happens. Probably will need test added to Until tests to ensure coverage is there.

Maybe tests/Storage/Queries/ChargeTest.php file needs to be updated to code coverage. Please let me know if there is anything I can do. Thank you

[gnikyt]

@nahid Tests pass, coverage didnt change as well, so thats OK.
Linting failed though, please see the Action for details.

[nahid]

@nahid Tests pass, coverage didnt change as well, so thats OK.
Linting failed though, please see the Action for details.

@osiset, I just pushed another commit with fixes, please check

[gnikyt]

@nahid Still is failing the lint.

Locally, you can vendor/bin/php-cs-fixer fix --diff --dry-run to see what needs to be changed, and vendor/bin/php-cs-fixer fix if you're OK with automatic changes.

[nahid]

@osiset please check the latest commits

[gnikyt]

Tests now pass, linting is good. @nahid

@Kyon147 I'm good with this one too if you are, seems like a feature thats been asked about before a few times.

[Kyon147]

Tests now pass, linting is good. @nahid

@Kyon147 I'm good with this one too if you are, seems like a feature thats been asked about before a few times.

Yeah looks good to me too

[gnikyt]

@nahid Please review once more, and if you're good we'll merge this in.

[nahid]

@nahid Please review once more, and if you're good we'll merge this in.

@osiset sure