Value resolver

[Lustmored]

This is extraction of value resolver from #91, without anything new and fancy 😸

[kbond]

Furthering on my comments from #91, 7ca9348 adds an important feature. Previously, only when persisting PendingFile's to your db, did they have their path generated. Now, any File that's persisted that does not match the mapped property's filesystem is saved in the same way.

What this potentially allows is:

public function updateProfileImage(
    User $user,
    EntityManagerInterface $em,

    #[UploadedFile(
        filesystem: 'temp'
        constraints: new ImageConstraint(maxWidth: 500, maxHeight: 500),
        errorStatus: 422, // this would be the default
    )]
    Image $image,
): Response {
    $user->setProfileImage($image);
    $em->flush();

    return new Response(...);
}

In the above specific scenario, it's probably better to use a PendingFile (and not save to the temp filesystem) but I'm thinking about live components where you'd want the file saved to a temporary filesystem to be able to pass back to the component (if some other field's validation failed).

[Lustmored]

@kbond Nice! Mentioned features makes most of my other PR pretty obsolete, so let's leave it aside for now.

I have tweaked and empowered value resolver quite a bit. Code is better organized in my opinion and is easier to extend. I have left storing to filesystem for now as I need to wrap my head around it. But the validation logic is now in place (it throws HttpException on failure).

Unfortunately the syntax for constraints in attributes is only available from PHP 8.1 forward ( https://www.php.net/releases/8.1/en.php#new_in_initializers ). I can try to skip some tests on PHP 8.0 or we can bump required PHP version (that'd be on par with Symfony 6.1+) - I leave the decision to you.

The problems I see with storing files to filesystems:

• I would prefer to store them on serialization, not initialization to not waste resources on files that'll be handled within a single request,
• I don't see a way to allow controller (mainly component) get the list of violations. Maybe in such a case the validation should be skipped altogether and done entirely on controller side.

Also I think it could be useful to have own HttpException for this case, so that it is possible to configure logging of it on a framework (see tests - right now they log errors during validation testing).

All in all it needs a bit more love, but is shaping nicely.

[Lustmored]

I think I have a take on this "store on serialization" concept. How about going this way:

Create a SerializablePendingFile class that would extend PendingFile (or decorate) and accept an additional parameter in constructor - a closure Closure(PendingFile $file): File that can be used to store file to a filesystem. An additional method (like save(): File) just calls the closure and returns the resulting file.

• If no serialization occurs - lifecycle of the object remains the same as usual, no side effects.
• The serialization of SerializablePendingFile is possible and it simply calls the closure first and then serializes the resulting file.

Creating such a closure in a value resolver phase is as simple as storing file (in fact it's the same logic, but wrapped in a closure).

(same for PendingImage)

What do you think of such approach?

[kbond]

What about allowing pendingfile to be lazy? Then we can wrap it in a serializedfile if you want to serialize it?

[Lustmored]

Wouldn't that store the file on access and not serialization? I admit I didn't analyze LazyNode/SerializableFile enough to answer that. Could you craft a code stub of how would that work?

[kbond]

Also I think it could be useful to have own HttpException for this case, so that it is possible to configure logging of it on a framework (see tests - right now they log errors during validation testing).

This is because throwing the HttpException generates logs?

I think we should leave it up to the user - they can configure this behaviour in framework/monolog config IIRC.

I've handled the test output in a few other libraries by setting a null logger: https://github.com/zenstruck/browser/blob/e46f25ae96f8a6b67ce2f547f6d4e6b24398a9a8/tests/Fixture/Kernel.php#L205

[kbond]

I think I have a take on this "store on serialization" concept. How about going this way:

I'm having a hard time understanding what you'd like to achieve with "store on serialization". Is it related to live components or another use-case?

I've been thinking about the lifecycle of a File/Image property on a live component:

1. User uploads file for this property
2. We always store to a temp filesystem
3. Validate
4. Regardless of validation failure, make the file available in the live twig template (so they can get the public/transform url)
5. Send the live response back (serializing the file as normal)

[Lustmored]

Thanks for the review! Regarding the serialization...

4. Regardless of validation failure, make the file available in the live twig template (so they can get the public/transform url)

Well, I completely forgot about this one when crafting the solution. Thank you for pointing this out, I admit to being wrong 👍 With that settled I think I will be able to finalize the PR next week without all this "store on serialize" madness 😸

[kbond]

Well, I completely forgot about this one when crafting the solution.

Ok, glad we're on the same page now!

It can wait for a later PR but the reason I was thinking of adding filesystem/namer properties to UploadedFile is so it could perhaps be used directly by live-components (we'd need to change the attribute to be allowed on properties of course).

[Lustmored]

It can wait for a later PR but the reason I was thinking of adding filesystem/namer properties to UploadedFile is so it could perhaps be used directly by live-components (we'd need to change the attribute to be allowed on properties of course).

Sure, they will be very simple to provide when we want to store file as soon as possible instead of as late as possible. I will add them 👍

[kbond]

Thinking we should have both UploadedFile and PendingUploadedFile to avoid one attribute from doing too much, wdyt?

[Lustmored]

I think I have done everything (but maybe more tests and docs obviously). I have indeed created separate attributes for pending file and stored one, so that distinction is obvious.

This is because throwing the HttpException generates logs?

I think we should leave it up to the user - they can configure this behaviour in framework/monolog config IIRC.

I have finally decided to add custom exception class. This is to actually allow configuring it's severity and on framework level. The config there is keyed by an exception class, so using HttpException would make it impossible to handle in separation of other HttpExceptions.

I also added the property to attributes. I'm unsure if that's required by live components (I need to catch up with live components development soon), but doesn't hurt to have it in place. When we'll have this one settled I'll jump back in to UX to see what happened in recent months. I think file uploads are still unhandled. If that's the case I'll start with frontend code only in hope that it'll be enough to use this library. Anything more will be added value 🎉

[kbond]

Really liking this - I can use this in my app right away!

I have finally decided to add custom exception class.

This is a good call. To clarify, by default, it will throw a 422, you can adjust the error code on a controller-by-controller basis or set your framework config to globally intercept/adjust?

[Lustmored]

I'm sorry it took so long to get back here.

This is a good call. To clarify, by default, it will throw a 422, you can adjust the error code on a controller-by-controller basis or set your framework config to globally intercept/adjust?

I went step ahead. The exception now extends UnprocessableEntityHttpException instead of HttpException, so that it inherits 422 status by default. This allowed me to remove status configuration from the attributes. This puts the feature on par with how other http exceptions usually work in Symfony (at least from my experience) - they can be configured on a framework level and tweaked as needed by exception listeners.

I think the last topic is the temporary file location that we can discuss in its topic

[kbond]

Just a few last minor things, then let's merge!

[Lustmored]

Just a few last minor things, then let's merge!

Both are commited and phpstan is made happy 😄

[kbond]

Awesome, thanks Jakub!