Representing a patch documents as data transfer objects

[acoulton]

Thanks for the excellent library :)

I'd quite like to be able to express a patch in PHP code rather than a JSON string, e.g. something like:

$FastJsonPatch = FastJsonPatch::fromJson($document);
$FastJsonPatch->apply(new JsonPatch(
    new Add(path: '/contacts-', value: ['name' => 'Jane']),
    new Replace(path: '/contacts/0/number', value: '+1 212 555 1212'),
    new Remove('/contacts/1'),
));

I think this could be done quite easily with a combination of simple value objects. I'd propose that these are separate objects to the existing blancks\JsonPatch\operations\PatchOperation, so that they have no behaviour or properties other than those that would be present in the JSON representation. For example:

namespace tbc;

final readonly class JsonPatch {
   public function __construct(
     public Operation ...$operations
  )
  // possibly implement iterator interface
}

final readonly class Remove implements Operation {
  public function __construct(
    public string $path
  )
}

Ideally the apply method typehint would be widened to public function apply(string|JsonPatch $patch): void so that the user can pass through their collection object.

This could either just stringify it and then pass it through to the existing json decoding in patchIterator. Or, I think it could keep it as an object and iterate it or it could keep it as an object and iterate it directly (the new DTOs just being used instead of the stdClass that currently comes from the decoded JSON patch).

The only real issue I can think of is picking a namespace / naming that is clear and concise for the end user, but does not conflict with the existing operation classes.

I would be very happy to put together a PR with an implementation for this if you'd be willing to consider it?

[blancks]

Hi @acoulton, glad this project has been useful, and thank you for having a look at the source code!

Serializing the DTOs back to JSON only to decode them again sounds like a waste of CPU cycles to me.

I'm absolutely fine with your proposal. The patchIterator method could be refactored to handle an object, maybe by breaking it down into a couple of more specific private methods to process the patch depending on whether it's a string or an instance of a class.

About the conflicting naming issue, I would workaround this by refactoring the existing operation class naming to OperationHandler. For example: AddHandler, CopyHandler, etc.

Those classes are not meant for external use. As long as we don't change the existing abstract PatchOperation class or the PatchOperationInterface, there should be zero risk of a backward-incompatible change.

Let me know what you think about this approach. And if you'd like to put together a PR, I would be absolutely happy with that!

[acoulton]

@blancks thanks for the response. I agree, keeping them as objects should be much more efficient and I'm pretty sure that can be done safely within the private implementation.

Renaming the existing classes to ...Handler sounds like a good solution for the naming issue. My only comment would be that I don't think they are explicitly tagged or documented as @internal so although they might not be meant for external use, there may be end-users who don't realise that?

I always lean quite cautious/strict on semver (maybe overcautious, but I've been bitten before 😆).

So I'd perhaps either do the rename as a 3.0 release (and mark them @internal at the same time), or do something like:

/**
 * @internal
 */
final class AddHandler extends Add {
  /* implementation */
}

/**
 * @deprecated see AddHandler
 */
class Add extends PatchOperation {
  /* empty class */
}

This will trigger phpstan warnings on the actual extends Add definitions which would have to be ignored, but usage of the child class would be fine - and it would guarantee safety for anyone using the library in an unexpected way.

What do you think?

Very happy to work on a PR.

[blancks]

@acoulton They're not tagged as @internal, indeed. But they're all final classes that are already registered for use inside the FastJsonPatch class.

The only scenario I can think of is if end-users somehow used them standalone, putting effort into figuring out the required data representation and dependencies to feed them.

This is something I thought to be extremely unlikely, in my honest opinion, but I totally see your point here.

The deprecation can break ci/cd pipelines since we are assuming people might have make use of those classes.

So I'm fine going strict with semver and bumping the major version 💪

[blancks]

I have created a dev branch that you can use as the base for the PR