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

BUGFIX: Allow choice of filter for resizing images #1513

Merged
merged 1 commit into from
Apr 4, 2017

Conversation

kitsunet
Copy link
Member

@kitsunet kitsunet commented Apr 3, 2017

The resize filter can have considerable impact on file size and
processing power needed to do the resize, it's therefore sensible
to give the user a choice in this.

Additionally removes copying the image as that is entirely unnecessary
and impacts memory usage profoundly.

As this is strongly impacting resource usage the filter is made
configurable as bugfix for now.
@mention-bot
Copy link

@kitsunet, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dfeyer, @robertlemke and @aertmann to be potential reviewers.

@kitsunet
Copy link
Member Author

kitsunet commented Apr 3, 2017

See comments on #1512 as well

Copy link
Member

@kdambekalns kdambekalns 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!

I checked a comparison site and could see no difference between any of the filters… so why not make FILTER_BOX the default?

@kitsunet
Copy link
Member Author

kitsunet commented Apr 3, 2017

Well, because as commented Gd only supported the undefined filter and imagine will throw an exception if you use anything else... I have no clue how to fix that on our side yet.

@kitsunet
Copy link
Member Author

kitsunet commented Apr 3, 2017

We could set box in setup when we figure out the driver...

@kdambekalns
Copy link
Member

Ah, overlooked that… Hm. Well, then maybe we can indeed add that to the setup step. But then no longer related to this PR. 👍

@robertlemke robertlemke merged commit 1116e52 into neos:2.3 Apr 4, 2017
@robertlemke robertlemke deleted the bugfix/resize-filter-choice branch April 4, 2017 08:18
@pankajlele
Copy link
Contributor

Site import/export is broken. Something related to this bugfix. Neos 2.3.12
An exception occurred while executing 'INSERT INTO typo3_media_domain_model_adjustment_abstractimageadjustment (persistence_object_identifier, position, width, height, maximumwidth, maximumheight, minimumwidth, minimumheight, ratiomode, allowupscaling, filter, imagevariant, dtype) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' with params ["929c104e-68c2-4250-a9a4-f594c17e1598", 20, 2508, 1254, null, null, null, null, "", 0, "undefined", "1ccc457e-5c8b-4062-a9e5-77a8696a0e63", "typo3_media_adjustment_resizeimageadjustment"]:
SQLSTATE[42S22]: Column not found: 1054 Unknown column 'filter' in 'field

@DrNik
Copy link

DrNik commented Apr 6, 2017

I can confirm the same error @pankajlele reported on Neos 3.0.3.
I can provide more details if needed.

Thanks,
N

@kdambekalns
Copy link
Member

@kitsunet Does the new property miss a Transient annotation!?

@kitsunet
Copy link
Member Author

kitsunet commented Apr 6, 2017

Checking that, could be the issue, but then something in Flow is broken because I am pretty sure that we excluded injected stuff at some point before.

@kitsunet
Copy link
Member Author

kitsunet commented Apr 6, 2017

So this change in itself is actually correct as it is...

Wrong is that here: https://github.com/neos/flow-development-collection/blob/master/Neos.Flow/Classes/Reflection/ReflectionService.php#L1609

properties with InjectConfiguration are not ignored for the (db) class schema...

@kitsunet
Copy link
Member Author

kitsunet commented Apr 6, 2017

@kitsunet
Copy link
Member Author

kitsunet commented Apr 6, 2017

Can confirm this is the cause and above is the fix for the 3.0 branch being broken.

@Benjamin-K
Copy link
Contributor

Is there a fix for 2.3 already?

@Benjamin-K
Copy link
Contributor

Ok, the fix for 3.0 also works for 2.3, so it should be merged there too.

@kitsunet
Copy link
Member Author

kitsunet commented Apr 7, 2017

Fix has been released in Flow 3.3.12 and 4.0.5 so an update should be fine to fix it.

@pankajlele
Copy link
Contributor

Great, thank you all! Fix is confirmed and working with Neos 2.3

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

Successfully merging this pull request may close these issues.

7 participants