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

Feature/allow imgproxy url modification #11

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

rtp-cgs
Copy link

@rtp-cgs rtp-cgs commented Mar 28, 2024

This allows to modify the imgproxyUrl before it gets returned. I also added a general method to add custom processing options to the ImgproxyUrl object.

I added a hook functionality since a proper signal doesn't work because the ThumbnailAspect class is an AOP aspect.
If you have any feedback please let me know.

Regards,
Cyrill

@rtp-cgs
Copy link
Author

rtp-cgs commented Apr 3, 2024

Hello there,

Do you have an first opinion if this will make it into your package soon or at all?
I don't want to stress I would just like to know if we can possibly use your package again soon instead of our fork.

Regards,
Cyrill

Copy link
Contributor

@christophlehmann christophlehmann left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! What do you think @hlubek ?

@@ -76,4 +76,9 @@ public function cacheBuster(string $cacheBuster)
{
$this->processingOptions[] = 'cb:' . $cacheBuster;
}

public function addProcessingOption($key, $value)
Copy link
Contributor

Choose a reason for hiding this comment

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

$key can be type-hinted with string

## How to modify the imgproxy url
If you need to modify the imgproxy url for your custom needs you can use the following hook:
1. Create a modifier class in this format:
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Add php here and <?php in the next line

}
```
2. Register your modifier in the settings:
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Add yaml here

@@ -109,6 +109,15 @@ public function generateImgproxyUri(JoinPointInterface $joinPoint): ?array

$expectedSize = ImgproxyBuilder::expectedSize($actualDimension, $targetDimension, $resizingType, $enlarge);

foreach ($this->settings['imgproxyUrlModifiers'] as $modifierClassName){
if(class_exists($modifierClassName)){
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO class_exists() and is_callable() should be removed here. When class does not exist or isn't callable, then i prefer to get an Exception and to know i made a mistake.

{
public function __invoke(ImgproxyUrl $url, ThumbnailConfiguration $configuration): void
{
// modify the ImgproxyUrl object
Copy link
Contributor

Choose a reason for hiding this comment

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

Add call to the new function here? $url->addProcessingOption('auto_rotate', 1)

@hlubek
Copy link
Contributor

hlubek commented Apr 4, 2024

@rtp-cgs Hi there! Thx for the PR and great to hear that the package is useful for you.

I looked at the code and would suggest a little different approach here: we could provide an interface for an ImgproxyUrlBuilder and inject that in the aspect and a default implementation DefaultImgproxyUrlBuilder that people could extend/replace and configure via Objects.yaml.

Maybe we could also use settings for some standard cases like processing options (e.g. extraProcessingOptions) but we have to take care that these are overridable from other packages.

@rtp-cgs
Copy link
Author

rtp-cgs commented Apr 4, 2024

Hello @hlubek

Thanks for the fast response and review.

I see why you suggest this approach, I would prepare a example matching your approach if that is okay for you?
For this to work I think I will need to set the $configuration variable from the aspect to the ImgproxyBuilder object.

I am not sure what the idea with the extraProcessingOptions option is, can you clarify this to avoid misunderstandings? Would you like to set static processing options like this:
extraProcessingOptions: ['zoom2:2', 'min-height:100']

@rtp-cgs
Copy link
Author

rtp-cgs commented Apr 24, 2024

Hey there Networkteam,

Sorry for the radio silence I was on holidays the last weeks. As requested I created a different solution in this branch / pull request: #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants