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

Refactor ExistsByHash to allow chaining query builder methods #121

Merged
merged 10 commits into from
Feb 11, 2024

Conversation

EriBloo
Copy link
Contributor

@EriBloo EriBloo commented Feb 7, 2024

Hi,

I recently switched one of our projects to this package. For previous package I had created validation rule which I have adapted to work with yours.

This validation rule is based on Laravel Exists rule and allows chaining constraints on query builder:

validator(
    ['id' => User::idToHash(1)],
    ['id' => new ExistsByHash(User::class)->where('name', 'Admin')],
);

I also added support for custom message and attribute so you could do:

validator(
    ['id' => User::idToHash(1)],
    ['id' => new ExistsByHash(User::class)],
    ['id.existsByHash' => 'Incorrect :attribute'],
    ['id' => 'user'],
);

to get Incorrect user validation message. If no custom rule was specified there is a fallback to Exists rule message.

@EriBloo EriBloo force-pushed the master branch 3 times, most recently from e55b673 to a4f1bce Compare February 7, 2024 21:34
@EriBloo
Copy link
Contributor Author

EriBloo commented Feb 8, 2024

I added one more commit to use qualified columns in byHash scope. Current version may cause Column 'id' in where clause is ambiguous errors when used with joins.

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8cbc50c) 100.00% compared to head (326def0) 100.00%.
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##              master      #121   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
- Complexity        46        53    +7     
===========================================
  Files              5         5           
  Lines            106       124   +18     
===========================================
+ Hits             106       124   +18     

@veelasky
Copy link
Owner

Ho @EriBloo thanks for the contributions, I noticed that if we push this, we should drop support for laravel 7-9, due to deprecation in Illuminate\Contracts\Validation\Rule

I wouldn't mind to release it under 4.x version, please also update the workflow to only include laravel 10.

…y#120)

Bumps [phpunit/phpunit](https://github.com/sebastianbergmann/phpunit) from 10.5.7 to 10.5.10.
- [Changelog](https://github.com/sebastianbergmann/phpunit/blob/10.5.10/ChangeLog-10.5.md)
- [Commits](sebastianbergmann/phpunit@10.5.7...10.5.10)

---
updated-dependencies:
- dependency-name: phpunit/phpunit
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…asky#118)

Bumps [orchestra/testbench](https://github.com/orchestral/testbench) from 8.20.0 to 8.21.1.
- [Release notes](https://github.com/orchestral/testbench/releases)
- [Changelog](https://github.com/orchestral/testbench/blob/develop/CHANGELOG-8.x.md)
- [Commits](orchestral/testbench@v8.20.0...v8.21.1)

---
updated-dependencies:
- dependency-name: orchestra/testbench
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@EriBloo
Copy link
Contributor Author

EriBloo commented Feb 11, 2024

Hi @veelasky. I work with Laravel 10 so I naturally pushed version compatible with it and forgot that current version is used by older Laravel as well. I don't mind refactoring it to the older Rule interface if you'd like, there is no reason to force next major version. Please tell me which approach is better for you ;)

Edit: I'm not sure how refactor would fit with custom message and attribute, would have to check it first.

@veelasky
Copy link
Owner

Hello @EriBloo,

Given the current situation, as there hasn't been a significant feature update, I aim to continue supporting lower Laravel versions, unless the deprecated class is removed or until a major feature such as prefix/suffix support is introduced.

@EriBloo
Copy link
Contributor Author

EriBloo commented Feb 11, 2024

I understand. But I'm not sure which approach you would like to take with this PR. Would you prefere me to refactor it to support Laravel versions 7-9 or release it under 4.x as is?

@veelasky veelasky merged commit ded5952 into veelasky:master Feb 11, 2024
10 checks passed
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.

3 participants