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.

Sign up for GitHub

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

Jump to bottom

Implement core support for | resize(width, height, options) filter #5231

Merged
merged 36 commits into from
Aug 22, 2020
Merged

Implement core support for | resize(width, height, options) filter #5231

merged 36 commits into from
Aug 22, 2020

Conversation

LukeTowers
Copy link
Contributor

@LukeTowers LukeTowers commented Aug 3, 2020 •
edited
Loading

Fixes #5059.

Example post processing plugin here: https://github.com/LukeTowers/oc-tinypng-plugin

Related PRs:

Replaces:

Remaining work:

  • Implement | imageHeight, | imageWidth filters
  • Implement support for the image resizing events when resizing File models
  • Cleanup inline documentation for ImageResizer class
  • Complete unit tests
  • Add support for a type: image column type

Unit tests to be completed:

  • Relative URLs for the four sources work (theme, plugin, media library, system file model)
  • Absolute URLs for the four sources work (theme, plugin, media library, system file model)
  • Accessing a file that should not be accessed (i.e something in storage/app/uploads/protected or another custom storage directory not explicitly allowed in the getAvailableSources() method) is denied
  • Generated resize images can't have their paths manipulated to break out of their intended target locations
  • Valid resizer URL gets correctly signed and validated
  • Invalid resizer URL (target URL doesn't match signed identifier) fails validation
  • | imageWidth and | imageHeight filters return the correct values
  • | resize() filter returns the original string if unable to create a valid image from it
  • Falsely values provided as width or height to the resizer result in the value of 0 internally
  • Images in a subfolder with a space character in the folder name in the media library are identified correctly (see Image not found if in subfolder with space in | media abwebdevelopers/oc-imageresize-plugin#30)
  • Resizer can correctly find images when October is installed in a subfolder (see Image not found when october is in a subfolder abwebdevelopers/oc-imageresize-plugin#28)

@mjauvin
Copy link
Contributor

mjauvin commented Aug 4, 2020

@LukeTowers so this is improving the getThumb() in that it can work on an image from the media library in ADDITION TO an image object ?

@LukeTowers
Copy link
Contributor Author

Sort of, this will rewrite the getThumb logic to be located in a more generic place and support handling File models, Media library items, and hopefully any other asset that the application can access (plugin / theme assets, etc)

Luke Towers added 11 commits August 6, 2020 18:18
Further work on the new resizing functionality
f56d1ee
Further WIP on resizer implementation, moving towards resizer object …
cdc45b0 
…instead of static methods on a helper class
More WIP, moved logic in the appropriate locations
7b9408e
More WIP on image resizer
263ae87 
Refactored resizer URLs to support custom disks, switched to using a signed version of the resized image's URL as the identifier for the configuration
required code from last commit
6926908
Support the resized URL as a route param instead of GET variable, ini…
a992fb4 
…tial WIP on supporting File models
Merge branch 'develop' into wip/image-resizing
b4dd255
Finished implementing support for retrieving thumb URLs from FileMode…
9cb8a08 
…ls, initial support for detecting completed resize operations
Reorganized the ImageResizer class
17664dc
Merge branch 'develop' into wip/image-resizing
f54b167
Play nicer with tests
04de6ef
LukeTowers
LukeTowers commented Aug 13, 2020
View reviewed changes
modules/system/classes/SystemController.php
$resizedUrl = ImageResizer::getValidResizedUrl($identifier, $encodedUrl);
if (empty($resizedUrl)) {
return response('Invalid identifier or redirect URL', 400);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bennothommo could we get a unit test for both of these code paths (correctly signed URL being validated vs incorrectly signed URL being rejected)?

@LukeTowers LukeTowers added this to the v1.0.469 milestone Aug 15, 2020
@bennothommo
Copy link
Contributor

Awesome work with this, @LukeTowers!

@LukeTowers
Copy link
Contributor Author

Thanks @bennothommo, I'm looking forward to getting this into production!

@bennothommo
Copy link
Contributor

bennothommo commented Aug 18, 2020 •
edited
Loading

@LukeTowers unless I'm mistaken, it appears the new events provided in this functionality won't fire for file models, so plugins extending this functionality wouldn't be able apply optimisations to uploaded files. Should these events be extended to those types of sources too?

EDIT: I mean the system.resizer.processResize and system.resizer.afterResize events.

@matteotrubini matteotrubini mentioned this pull request Aug 19, 2020
Closed
@mjauvin
Copy link
Contributor

mjauvin commented Aug 21, 2020

@LukeTowers I never got a response to my comment about the beforeResize event being documented but not fired anywhere...

@LukeTowers
Copy link
Contributor Author

@mjauvin sorry, didn't see that. After thinking about it I couldn't find a legitimate use case for it so I didn't implement it. Still need to clean up the inline docs.

@LukeTowers LukeTowers marked this pull request as ready for review August 21, 2020 22:11
LukeTowers added a commit to octobercms/docs that referenced this pull request Aug 22, 2020
Merge pull request #472 from octobercms/wip/image-resizing
43165e8 
Document new Image Resizing functionality. Related: octobercms/october#5231
@LukeTowers LukeTowers merged commit c1c728e into develop Aug 22, 2020
@LukeTowers LukeTowers deleted the wip/image-resizing branch September 4, 2020 18:44
bennothommo added a commit to octobercms/library that referenced this pull request Sep 15, 2020
Use correct spelling for "normalize" in PathResolver
e2ed5fb 
Refs: octobercms/october#5231 (comment)
@everyx everyx mentioned this pull request Dec 15, 2020
Closed
@PubliAlex
Copy link

Hello,

Is there some documentation somewhere about this new core filter ? I looked for some doc on the octobercms website without success. Thank you

@LukeTowers
Copy link
Contributor Author

Yes, it'll be added to the marketplace site when 1.1 is available on the marketplace, until then use the RainLab.Docs plugin or look at the docs repo directly: https://github.com/octobercms/docs/blob/master/services-image-resizing.md, https://github.com/octobercms/docs/blob/master/markup-filter-resize.md, https://github.com/octobercms/docs/blob/master/markup-filter-image-width.md, https://github.com/octobercms/docs/blob/master/markup-filter-image-height.md

@lieszkol lieszkol mentioned this pull request Jul 23, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bennothommo bennothommo bennothommo left review comments

Assignees
No one assigned
Labels
Status: Completed Type: Enhancement
Milestone
v1.1.0
Development

Successfully merging this pull request may close these issues.

Add core support for the | resize(width, height, options) filter
6 participants
@LukeTowers @mjauvin @bennothommo @PubliAlex @alxy @daftspunk