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

Dispatch an event when an image is resized #5

Closed
wants to merge 4 commits into from

Conversation

leofeyer
Copy link
Member

@leofeyer leofeyer commented Aug 3, 2016

This PR adds the contao.image.resize_image event, which is dispatched when an image is resized (Resizer::executeResize).

I have chosen resize_image instead of just resize so the class is called ResizeImageEvent instead of ResizeEvent and the constant is called ContaoImageEvents::RESIZE_IMAGE instead of ContaoImageEvents::RESIZE. I can change this though if you want.

@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-0.7%) to 94.541% when pulling 70d20e79f29e35477e9e3440972d7bee375ac24d on feature/event-listener into 21636e8 on master.

*
* @param ImageInterface $image
*/
public function setResizedImage(ImageInterface $image)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should allow null here too, so that an event listener can remove a resized image that a previous event listener added.

@ausi
Copy link
Member

ausi commented Aug 3, 2016

LGTM. I added some comments and I think we should add "symfony/event-dispatcher": "~2.8|~3.0" to composer.json.

ResizeCalculatorInterface $calculator,
Filesystem $filesystem,
$path,
EventDispatcher $eventDispatcher
Copy link
Member

Choose a reason for hiding this comment

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

How about making the event dispatcher optional and set it via a setEventDispatcher method?

@leofeyer
Copy link
Member Author

leofeyer commented Aug 4, 2016

I have updated everything according to your comments. Also, I have moved the event dispatcher routines to the top of the executeResize() method, because the resized image might have a different $path and there is no need to mkdir() a directory which is not used later on.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.9%) to 94.372% when pulling eb196f3 on feature/event-listener into 21636e8 on master.

@@ -18,7 +18,8 @@
"require-dev": {
"friendsofphp/php-cs-fixer": "~1.8",
"phpunit/phpunit": "~4.5",
"satooshi/php-coveralls": "~0.6"
"satooshi/php-coveralls": "~0.6",
"symfony/event-dispatcher": "~2.8|~3.0"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t it go into "require"? We are using the interface which could change in a major version.

Copy link
Member Author

Choose a reason for hiding this comment

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

But the event dispatcher is now optional, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but we have to ensure the correct version of the interface we are using. The library might be incompatible with EventDispatcherInterface version 4.0.

@coveralls
Copy link

coveralls commented Aug 4, 2016

Coverage Status

Coverage decreased (-0.9%) to 94.372% when pulling eb196f3 on feature/event-listener into 21636e8 on master.

ausi added a commit that referenced this pull request Aug 4, 2016
@ausi
Copy link
Member

ausi commented Aug 4, 2016

Merged in 5410142

@ausi ausi closed this Aug 4, 2016
@leofeyer leofeyer deleted the feature/event-listener branch August 4, 2016 07:50
ausi added a commit that referenced this pull request Aug 4, 2016
This reverts commit 5410142, reversing
changes made to bb80a5f.

Conflicts:
	src/Event/ResizeImageEvent.php
	src/Resizer.php
	src/ResizerInterface.php
	tests/ResizerTest.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants