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

Remove image cache #113

Merged
merged 2 commits into from
Aug 4, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ There's a frood who really knows where his towel is.
2.10.2 (unreleased)
^^^^^^^^^^^^^^^^^^^

- Nothing changed yet.
- Remove useless scale caching on the request as it seems to be causing colateral issues (closes `#109 <https://github.com/collective/sc.social.like/issues/109>`_).
[rodfersou]


2.10.1 (2017-08-02)
Expand Down
92 changes: 38 additions & 54 deletions sc/social/like/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,71 +4,55 @@
from Products.CMFPlone.utils import safe_hasattr
from sc.social.like.logger import logger
from urlparse import urlparse
from zope.annotation.interfaces import IAnnotations
from zope.globalrequest import getRequest
from zope.interface import Invalid


def get_images_view(context):
request = getRequest()
key = 'cache-view-' + str(context)
cache = IAnnotations(request)
value = cache.get(key, None)
if not value:
view = context.unrestrictedTraverse('@@images', None)
field = 'image'
if view:
fields = ['image', 'leadImage', 'portrait']
if IBaseContent.providedBy(context):
schema = context.Schema()
field = [f for f in schema.keys() if f in fields]
if field:
field = field[0]
# if a content has an image field that isn't an ImageField
# (for example a relation field), set field='' to avoid errors
if schema[field].type not in ['image', 'blob']:
field = ''
value = (view, field) if (view and field) else (None, None)
cache[key] = value
return value
view = context.unrestrictedTraverse('@@images', None)
field = 'image'
if view:
fields = ['image', 'leadImage', 'portrait']
if IBaseContent.providedBy(context):
schema = context.Schema()
field = [f for f in schema.keys() if f in fields]
if field:
field = field[0]
# if a content has an image field that isn't an ImageField
# (for example a relation field), set field='' to avoid errors
if schema[field].type not in ['image', 'blob']:
field = ''
return (view, field) if (view and field) else (None, None)


def get_content_image(context,
scale='large',
width=None,
height=None):
request = getRequest()
modification = context.ModificationDate()
key = 'cache-{0}-{1}-{2}-{3}-{4}'.format(
context, modification, scale, width, height)
cache = IAnnotations(request)
img = cache.get(key, None)
if not img:
view, field = get_images_view(context)
if view:
view, field = get_images_view(context)
img = None
Copy link
Member

Choose a reason for hiding this comment

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

there's no need to initialize this variable here.

Copy link
Member Author

@rodfersou rodfersou Jul 19, 2017

Choose a reason for hiding this comment

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

yes, there is. In some case running in production the view or sizes object is None, and the method try to return a variable not initialized, resulting in an exception.

Copy link
Member

Choose a reason for hiding this comment

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

that variable is already set in lines 36, 52 and 54; there is no need to initialize it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hvelarde are you considering when view variable is None? I swear to you I did this because there was an exception without it, and this fixes the exception. you can check in the logs.

Also I don't want to move too much code in this PR, we have other issue for this.

if view:
try:
sizes = view.getImageSize(field)
except AttributeError:
sizes = img = None
if sizes == (0, 0) or sizes == ('', ''):
# this avoid strange cases where we can't get size infos.
# for example if the loaded image in a news is a bmp or a tiff
return None
if sizes:
kwargs = {}
if not (width or height):
kwargs['scale'] = scale
else:
new = (width, height)
width, height = _image_size(sizes, new)
kwargs['width'] = width
kwargs['height'] = height
kwargs['direction'] = 'down'
try:
sizes = view.getImageSize(field)
except AttributeError:
sizes = img = None
if sizes == (0, 0) or sizes == ('', ''):
# this avoid strange cases where we can't get size infos.
# for example if the loaded image in a news is a bmp or a tiff
return None
if sizes:
kwargs = {}
if not (width or height):
kwargs['scale'] = scale
else:
new = (width, height)
width, height = _image_size(sizes, new)
kwargs['width'] = width
kwargs['height'] = height
kwargs['direction'] = 'down'
try:
img = view.scale(fieldname=field, **kwargs)
except (AttributeError, TypeError):
img = None
cache[key] = img
img = view.scale(fieldname=field, **kwargs)
except (AttributeError, TypeError):
img = None
return img


Expand Down