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

getIcon indexer returns True for a folderish item with any children with id "image" #3916

Closed
frapell opened this issue Mar 5, 2024 · 6 comments

Comments

@frapell
Copy link
Member

frapell commented Mar 5, 2024

The issue is happening here

@indexer(Interface)
def getIcon(obj):
"""
geticon redefined in Plone > 5.0
see https://github.com/plone/Products.CMFPlone/issues/1226
reuse of metadata field,
now used for showing thumbs in content listings etc.
when obj is an image or has a lead image
or has an image field with name 'image': true else false
"""
return bool(getattr(obj.aq_base, "image", False))

In order to see the problem:

  1. Create a folder with any name (e.g. Test Folder), this results in /Plone/test-folder
  2. Inside this new folder, add another item, can be a page or another folder, and name it specifically "Image" so its id ends up being "image". this results in /Plone/test-folder/image

At this point, if you go check the indexed folder from step 1 (/Plone/test-folder) you will notice that the getIcon metadata is True.

This causes issues, such as plone/plone.namedfile#156

@davisagli provided a bit more context in this other issue, however I believe this must be fixed here, and have getIcon to properly return True only when the indexed item does have an image.

@frapell
Copy link
Member Author

frapell commented Mar 5, 2024

I have included a test that triggers the bug. I am not sure how to properly fix this... I don't think we want to import behaviors such as https://github.com/plone/plone.app.contenttypes/blob/master/plone/app/contenttypes/behaviors/leadimage.py in here?

@frapell frapell self-assigned this Mar 5, 2024
@davisagli
Copy link
Member

@frapell How about, get the image attribute like it does currently, but then add a check that the value is an instance of NamedImage?

@frapell
Copy link
Member Author

frapell commented Mar 5, 2024

@davisagli Yeah, it is either that or maybe adding image to the list of reserved words... what about checking if it provides plone.namedfile.interfaces.IImage ?

@davisagli
Copy link
Member

@frapell the IImage interface check should work

@yurj
Copy link
Contributor

yurj commented Mar 6, 2024

So to implement the "icon" for a content, a NamedImage or an object implementing the plone.namedfile.interfaces.IImage interface should be returned when calling 'image' (attribute or function) on it?

@frapell
Copy link
Member Author

frapell commented Mar 6, 2024

@yurj I am not sure why this indexer is called getIcon while it should really be called hasImage or something like that... it will only return True or False if the item has an image. You can see the referenced PR in the comment (#1226) it is almost 10 years old, so you need to dig some deeper to find the origin of it, maybe at some point it was used for icons.
Content icons these days are managed through css AFAIK...

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

No branches or pull requests

3 participants