mtarld / api-platform-ms-bundle

A set of tools to use Api Platform in a microservice context
MIT License
66 stars 8 forks source link

FeatureRequest: Allow optional PATCH (additional to PUT) #53

Closed art-cg closed 2 years ago

art-cg commented 3 years ago

Hi. Is there a reason why this bundle is useing PUT instead of PATCH for Updates? Are there any disadvatages of PATCH?

We use actual only PATCH in our APIs. We habe now to decide if we replace it with PUT, or create a PR that allows PUT and PATCH for this bundle. => Allow optional PATCH (additional to PUT)

Any advise on this topic?

Not tested, just to give some context.

src/HttpRepository/AbstractMicroserviceHttpRepository.php `public function update(ApiResourceDtoInterface $resource, array $additionalQueryParams = []): ApiResourceDtoInterface { ...

    try {
        #$response = $this->request('PATCH', $this->buildUri($iri, $additionalQueryParams), $resource, 'application/merge-patch+json', 'json');
        $response = $this->request('PUT', $this->buildUri($iri, $additionalQueryParams), $resource, null, 'json');`

Greetz

mtarld commented 3 years ago

Hi @art-cg!

Yes indeed, both PUT and PATCH methods must be supported. Actually, I created this bundle because of a project, and in that project, they used the PUT method to update resources. But you're right PATCH is legitimate, and moreover, I think that it must be the default one.

I think that we can introduce a container configuration. Something like a switch to specify if we wanna use PATCH or PUT (with PUT to default in 0.x and with PATCH to default in 1.x to handle BC) We'll also need to know if it's generic or if it's specific to each microservice.

Are you up for a PR? :slightly_smiling_face:

ismail1432 commented 3 years ago

Hey guys 👋

Instead of dealing with container configuration what about introduce a new method patch method and a new method put inside the repository. We have to depreciate the current update method to remove it in favor of put in the next major version.

mtarld commented 3 years ago

Yep, that's a good idea! Much simpler to implement. On the other hand, I don't wanna leak the verb into the method's name.

Maybe we can find appropriate names though? Something like:

Any other ideas are welcome!

art-cg commented 3 years ago

Thanks for the fast response. Please let me know how i should implement it. The main goal should be no BC break.

ismail1432 commented 3 years ago

partialUpdate is a good idea

mtarld commented 3 years ago

Nice! So let's do that!

@art-cg, the way to do it quite straightforward:

Sounds good to you?

art-cg commented 3 years ago

Great. I will work on a PR

mtarld commented 2 years ago

Close it as #56 is merged