search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
249 stars 188 forks source link

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

Closed frapell closed 7 months ago

frapell commented 7 months ago

The issue is happening here https://github.com/plone/Products.CMFPlone/blob/72ec2cad51e3096b043f79b1777f6e452f83afdf/Products/CMFPlone/CatalogTool.py#L226-L237

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 https://github.com/plone/plone.namedfile/issues/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 commented 7 months ago

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?

davisagli commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

@frapell the IImage interface check should work

yurj commented 7 months ago

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 commented 7 months ago

@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 (https://github.com/plone/Products.CMFPlone/issues/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...