Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add a request macro #1

Merged
merged 2 commits into from
Oct 22, 2022
Merged

Add a request macro #1

merged 2 commits into from
Oct 22, 2022

Conversation

grantholle
Copy link
Contributor

This adds a request macro that can be used to transform the underlying request data. It cuts out some of the extra syntax and transforms the request data.

If you choose to merge this, it would be super helpful for you to add the hacktoberfest-accepted tag!

This adds a request macro that can be used to transform the underlying request data.
@surgiie
Copy link
Owner

surgiie commented Oct 21, 2022

Hi,

Thanks for making a PR for this. I was planning to eventually write out a provider to integrate better with the framework. The macro on the request is a great idea. Ill give this a further review and test this later this afternoon/evening.

@surgiie surgiie self-requested a review October 22, 2022 00:49
Copy link
Owner

@surgiie surgiie left a comment

Choose a reason for hiding this comment

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

Things appear to be working as expected. Thank you for your contribution and adding this.

Ive got some minor edits I want to make to some of the code (typos, minor polishes) and plan to release a minor version bump of v0.2.0 soon.

Transformer::unguard();
// This is kind of a dirty way to add the macro
// without including orchestra/testbench
(new \Surgiie\Transformer\TransformerServiceProvider(''))
Copy link
Owner

Choose a reason for hiding this comment

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

Im okay with this for now since we're still able to test the functionality of the package without orchestra/testbench. Will leave it as a future consideration to pull in orchestra/testbench if the package grows to the point where functionality cannot be tested without it.

Request::macro('transform', function (array $input, array $transformers = null): array {
$localTransformers = $transformers ?? $input;
$input = is_null($transformers)
? (method_exists($this, 'validated')
Copy link
Owner

@surgiie surgiie Oct 22, 2022

Choose a reason for hiding this comment

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

I think using validated if the request object is a FormRequest is okay but does have a silent omission of transformation if a validation rule is not defined in the form request. Example:

If you do something like: $formRequest->transform(['name' => 'trim|ucfirst'])

but dont have a rule defined in the form request, i.e:

public function rules(){
    return [
        'name'=>['...'] 
    ];
}

That data is not transformed, and it will not be present in the transformed data array returned from transform which may cause some confusion, however I think im okay with this for now and just pointing it out, Ill make a note of this on the README in a future commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's a valid point. It's a bit presumptuous.

@surgiie surgiie merged commit 946d7f8 into surgiie:master Oct 22, 2022
@surgiie
Copy link
Owner

surgiie commented Oct 22, 2022

This has been released in v0.2.0.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants