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

Remove validator overwrite #63

Merged
merged 7 commits into from
Oct 1, 2024

Conversation

aedart
Copy link
Contributor

@aedart aedart commented Dec 28, 2022

This PR extracts validation logic out of the custom validation and into its own class, which is then installed as a validation extension, via the service provider.

The custom validator has now been marked as deprecated, because it is really not needed. The validation rule is sufficient.
Lastly, the unit tests have also been adapted to no longer rely on the custom validator instance.

Related

Additional Context

Recently I found myself in a bad situation - I needed to overwrite my application's Validator instance to manually fix an issue. To my great surprise, I found that this package already had overwritten the validator and caused me some frustration (I was not aware of this behaviour).
In any case, I thought that I would take a look at how your package could be improved, to avoid the above mentioned situation.

The is no need for a custom validator instance. It is bad practice for 3rd party packages to define rules this way.
Also, removed the custom resolver  - the logic that is overwriting an application's  validator instance.
Helper is responsible for creating validator instance with desired validation rules...
@aedart
Copy link
Contributor Author

aedart commented Dec 28, 2022

Note: I'm not sure what/why code-coverage is failing. @sunspikes, if you approve of the changes, can you please take over the PR and fix eventual coverage related issues?

@aedart
Copy link
Contributor Author

aedart commented Feb 16, 2023

@sunspikes are you still around?
Just wondering if you had any chance to review this PR and maybe make it a part of your package?

@marcovo-peercode
Copy link

marcovo-peercode commented Aug 10, 2023

@sunspikes This change is in line with how new rules should be added to laravel projects, and as such would be a huge improvement to this package. Could you please consider merging this PR, or if this package is abandoned, marking it as such so someone else can take over?

@aedart
Copy link
Contributor Author

aedart commented Aug 15, 2023

To anyone that might come across this PR. Due to lack of activity from the author, I was forced to create an alternative. I hold no ill intention towards the author and hope that he may become active again in the future.

@KristianI
Copy link

Voting for this PR to be merged 👍🏼 cc @sunspikes

@bradietilley
Copy link

Should this package be archived or should this PR be merged? 👀

@aedart
Copy link
Contributor Author

aedart commented May 21, 2024

@bradietilley I can see that the author has recently been active on this package - he added support for Laravel v11 (at least in the composer file). But I cannot say why this PR has not received any attention.

@sunspikes
Copy link
Owner

Thank you for this, currently Im mostly disconnected, I will check this and merge asap, sorry for the delay!

@sunspikes sunspikes merged commit 9aa898e into sunspikes:master Oct 1, 2024
@sunspikes
Copy link
Owner

Thank you @aedart and sorry for the late merge

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.

5 participants