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

Glide integration #49

Closed
wants to merge 12 commits into from
Closed

Glide integration #49

wants to merge 12 commits into from

Conversation

Lustmored
Copy link
Contributor

@Lustmored Lustmored commented Aug 17, 2022

Given the simplicity of features in this library I found it simple to add glide server integration. Given glide popularity I believe it is worth to be available out of the box.

It is implemented by UrlBuilderGlideUrlFeature, which takes Glide own UrlBuilder as an argument and uses it to generate URLs for images (with URL signing when configured and all that stuff).

Usage is as simple as calling $image->glideUrl(['w' => 100, 'h' => 100]).

I chose to add a new method because it is beneficial to be able to call both url() and glideUrl() on the same image and get different results (path to original image versus path to "glided" image).

@kbond
Copy link
Member

kbond commented Aug 17, 2022

Excellent! Glide integration was indeed on my radar.

One thing I'd like to do is make this feature more generic. Here's what I'm thinking:

namespace Zenstruck\Filesystem\Feature;

use Zenstruck\Filesystem\Node\File\Image;
use Zenstruck\Uri;

interface ImageTransformationUrl
{
    public function imageTransformationUrlFor(Image $file, mixed $options = []): Uri;
}

Then in Zenstruck\Filesystem\Node\File\Image, we'll add a generic transformationUrl() method.

My reasoning here is there are several implementations we could have:

  1. Glide (this PR)
  2. Route - some custom route that does your own thing (I use this in my app along with Imagine)
  3. Cloudinary (cloud based image transformation service)
  4. Imgix (similar to Cloudinary)

I wouldn't want to have a new Image::*url() for every implementation.

WDYT?

BTW, is transformation the best term to use here?

@kbond
Copy link
Member

kbond commented Aug 17, 2022

I'm not super familiar with Glide but one thing that I think we'd need as well is some kind of Filesystem<->Glide bridge to be able to use Zenstruck\Filesystem's with glide. Not sure how this would work yet.

@Lustmored
Copy link
Contributor Author

I'm not super familiar with Glide but one thing that I think we'd need as well is some kind of Filesystem<->Glide bridge to be able to use Zenstruck\Filesystem's with glide. Not sure how this would work yet.

I think we actually don't. Glide is already built on top of Flysystem and requires configuration by it's own. And when it's configured all that we need is an instance of UrlBuilder to work with it.

About making this feature more generic - I'm all in :D this is somehow on par with image manipulation interface I have in my mind right now (expect PR soon 😉 ).

And about the naming. I would love for this to be short, because long method names look weird in Twig templates to me. How about calling it "thumbnailers" and use thumbUrl on image class? Those transformators are way more powerful than just thumbnailing, yet this is the main usage for them.

@kbond
Copy link
Member

kbond commented Aug 18, 2022

I think we actually don't. Glide is already built on top of Flysystem and requires configuration by it's own. And when it's configured all that we need is an instance of UrlBuilder to work with it.

But this would require configuring flysystem twice, no? Once for glide and once for this library.

And about the naming. I would love for this to be short, because long method names look weird in Twig templates to me. How about calling it "thumbnailers" and use thumbUrl on image class? Those transformators are way more powerful than just thumbnailing, yet this is the main usage for them.

I think I'd still like to call the feature interface ImageTransformationUrl or maybe TransformImageUrl then maybe Image::transformUrl() is a bit shorter? I'd be fine with a second alias method of thumbUrl() - you're right, it is the most common usage.

(naming stuff well is an obsession of mine - maybe a sickness tbh...)

I'm all in :D this is somehow on par with image manipulation interface I have in my mind right now

Did you take a look at #15? I'd really like to have this library able to transform the images itself (using Imagine or Intervention - they are the most popular libraries I think).

I was thinking something like:

$image = $filesystem->image('some/image.png)
    ->transform(function(ImagineImage|IterventionImage $image): ImagineImage|IterventionImage {
        return $image->transformation1()->transformation2();
    })
;

$image; // ?? \SplFileInfo

My idea was this transform() method would return an SplFileInfo that can be written to a filesystem. @weaverryan expressed some concern that it could be unexpected that it return an SplFileInfo object and not manipulate the image in place on the filesystem. I agree but at the same time, I don't think we want to manipulate the image in-place... Or if we do, another method that does that. We'd want a way to create multiple manipulated images from a base image.

@Lustmored
Copy link
Contributor Author

I have made changes to the feature and classes names as suggested. Hopefully they are generic enough right now :) I didn't change Sf bridge configuration for now as I'm unsure on how to get this right, so input welcome :)

But this would require configuring flysystem twice, no? Once for glide and once for this library.

The way I see using this lib, flysystem and glide it does not, but maybe I'm missing something. I see all of those working in a tandem like this:

  • Flysystem is integrated in the project by flysystem-bundle and this is where all flysystem-specific configuration goes (maybe we should add it to suggested in composer for better Symfony integration?)
  • Glide is configured in an endpoint (regular Sf controller) that auto-wires required Flysystem adapters by the bundle and configures Glide as desired.
  • Glide own UrlBuilder needs minimal setup (which most projects probably do already), like this one:
$services->set(UrlBuilder::class)
             ->factory([UrlBuilderFactory::class, 'create'])
             ->arg('$baseUrl', 'https://%domain.img%/')
    ;
  • This library configures filesystems based on adapters already configured by flysystem-bundle.

@kbond
Copy link
Member

kbond commented Aug 19, 2022

This library configures filesystems based on adapters already configured by flysystem-bundle.

Ah, ok, I understand now. My intention was to have this library/bundle as an alternative to flysystem-bundle but didn't like that it was required to choose one or the other.

That being said, this bundle can be used standalone but I will look into how we can better integrate with flysystem-bundle (#53).

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

Looks good! Just a few minor things.

src/Filesystem/Node/File/Image.php Outdated Show resolved Hide resolved
src/Filesystem/Node/File/Image.php Outdated Show resolved Hide resolved
src/Filesystem/Node/File/Image.php Outdated Show resolved Hide resolved
src/Filesystem/Node/File/Image.php Outdated Show resolved Hide resolved
@kbond
Copy link
Member

kbond commented Aug 19, 2022

I have made changes to the feature and classes names as suggested. Hopefully they are generic enough right now :) I didn't change Sf bridge configuration for now as I'm unsure on how to get this right, so input welcome :)

I think the current configuration is fine, assuming we have good integration with flysystem-bundle. I do still think there should be a possibility of using this bundle and glide standalone. Something like:

zenstruck_filesystem:
    filesystems:
        images: 'some.dsn'
        glide.cache: 'some.dsn'

    glide_server:
        source: images # filesystem name from above
        cache: glide.cache # filesystem name from above
        # ... other ServerFactory options

Basically create a ServerFactory service. Maybe a route to be included with this bundle.

But I'll save this for later.

@kbond
Copy link
Member

kbond commented Aug 19, 2022

BTW, don't worry about getting bogged down fixing failing sca/cs jobs. Tests pass so that's fine.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Lustmored and others added 7 commits August 20, 2022 10:37
Co-authored-by: Kevin Bond <kevinbond@gmail.com>
Co-authored-by: Kevin Bond <kevinbond@gmail.com>
Co-authored-by: Kevin Bond <kevinbond@gmail.com>
Co-authored-by: Kevin Bond <kevinbond@gmail.com>
…l.php

Co-authored-by: Kevin Bond <kevinbond@gmail.com>
Co-authored-by: Kevin Bond <kevinbond@gmail.com>
Co-authored-by: Kevin Bond <kevinbond@gmail.com>
@kbond
Copy link
Member

kbond commented Aug 29, 2022

Let's drop the wiring in the Symfony bundle for now. Once we have more implementations of this feature available, we can re-evaluate how it should be configured.

My plan is to have a features config for each filesystem to add whatever you want and act as an escape hatch for adding this without an explicit config option (#57):

zenstruck_filesystem:
    filesystems:
        public:
            dsn: ...
            features:
                Zenstruck\Filesystem\Feature\TransformImageUrl: some.service

@Lustmored
Copy link
Contributor Author

I am closing this in favor of #79

@Lustmored Lustmored closed this Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants