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

Framework agnostic approach #38

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

denisvanmorgan
Copy link

Hi,

we are using your package in our project to validate crypto addresses on our infrastructure layer for our request DTOs. Thank you for maintaining this project, so far we were really happy to work with it.

We decided to upgrade our dependencies but this one kept throwing composer error messages from version 2.0.X. So I checked it and it seems that the package is tightly coupled with laravel/framework package. Which is quite unfortunate if you want to use it as a standalone package in your non-framework project or project where you use Symfony, Nette, Yii, CakePHP etc. I must say that it's not desirable to fetch whole laravel/framework lib because of few lines of code.

So I made some changes to make things more flexible and make it possible for other people to use the package however they want.

  1. I removed Laravel dependency
  2. Refactored methods from Str class to native implementation
  3. Removed check if app is in production env or not and added optional parameter to static factory method in Validator (let's improve DX experience, there might be a case when someone wants to use testnet on production environment or may have different validator instances, etc.)

So it can be used right now like this:

Validator::make('BTC', app()->isProduction())
or
Validator::make('BTC', $this->featureFlag->isTestnet())
etc, etc.

  1. Resolve config in a php native way (simply require the config and work with returned config array)
  2. Added optionable config parameter for developers to extend/replace package config
  3. Removed unused usages
  4. Added missing usages

If you have any questions feel free to hit me up.

denismacak added 2 commits January 23, 2024 13:44
…amework library, small docblock updates, removed unused imports
…amework library, small docblock updates, removed unused imports
@@ -45,13 +41,4 @@ private function getDriverOptions(bool $isMainNet): array
?: $this->mainnet
?: [];
}

public static function __set_state(array $state): DriverConfig
Copy link
Author

Choose a reason for hiding this comment

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

seems like there is not usage for this magic method in a code anymore. feel free to correct me if I missed something

}

public static function make(CurrencyEnum $currency): Validator
public static function make(CurrencyEnum $currency, bool $isMainnet = true, ?array $config = null): self
Copy link
Author

Choose a reason for hiding this comment

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

since this factory method copies current construct method I was thinking if it would make sense to remove it. But I left it here for backwards compatibility reasons. Maybe we can docblock it with @deprecated to encourage developers to use constructor instead?

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.

1 participant