Skip to content

Conversation

@adesege
Copy link
Contributor

@adesege adesege commented Jan 9, 2022

This PR extends wallet holder_id property to support string type.

In my use case, user id and item id are uuid strings.

@adesege adesege force-pushed the add-support-for-wallet-holder-string branch 4 times, most recently from 865cb6a to 8c4abe7 Compare January 9, 2022 23:24
@rez1dent3
Copy link
Member

rez1dent3 commented Jan 10, 2022

@adesege Maybe I'll pick up this MR, but definitely not in the near future.

  1. The package supports php 7.4;
  2. Contract and backward compatibility violated. This is only a major version;

At the moment, I'm throwing it on the back burner.

@rez1dent3 rez1dent3 marked this pull request as draft January 10, 2022 16:18
@adesege
Copy link
Contributor Author

adesege commented Jan 10, 2022

Hi @rez1dent3, thanks for the comment.

  1. Union types was introduced in PHP 8.0. I can use psalm instead if you are okay with it.
  2. I tried fixing this in my code but couldn't cos of the contract. I'm not sure it broke backward compatibility cos the methods accept integer or string. Those using integers will still be able to use it as expected.

I'm sure there are other people who use uuid strings as their primary id as well.

Let me know if there is a temporary way of solving this without extending core.

Thanks.

@rez1dent3
Copy link
Member

@adesege There is another suggestion. I suggest postponing your pull request until laravel 9 is released. The new version will remove support for php 7.4 and I was going to remove support for old laravel versions in laravel-wallet 8+.

@rez1dent3
Copy link
Member

@adesege It was based #424 on your pull request. When the release of laravel 9 comes out and all the libraries are pulled up, I will release the release WITHOUT support for laravel 8.x. At the moment, I can only suggest using the dev version.

composer req bavix/laravel-wallet:dev-develop

@adesege
Copy link
Contributor Author

adesege commented Jan 10, 2022

Great! Laravel 9 will be released by Jan. 25th. I can wait till then. Thanks.

@adesege
Copy link
Contributor Author

adesege commented Jan 13, 2022

Hey @rez1dent3, I've added another commit to extend Transfer DTO assembler and contract.

@rez1dent3 rez1dent3 changed the base branch from master to develop January 14, 2022 13:22
@rez1dent3 rez1dent3 marked this pull request as ready for review January 14, 2022 13:23
@rez1dent3 rez1dent3 merged commit d3f99a8 into bavix:develop Jan 14, 2022
@adesege
Copy link
Contributor Author

adesege commented Jan 26, 2022

@rez1dent3 I've opened another PR to add string support for wallet id in transaction dto interface here #427

@rez1dent3
Copy link
Member

@adesege hello.
released version with laravel 9 support.
It included part by uuid.
tag 8.0.0

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.

2 participants