plone / plone.outputfilters

Provides a framework for registering filters that get applied to text as it is rendered.
6 stars 15 forks source link

Overriding captioned image view with new template may cause traceback: #40

Closed flipmcf closed 4 years ago

flipmcf commented 4 years ago

After investigating, I considered #39 to be user error, because a customization caused the package to malfunction.

However, it does raise the question - should a custom captioning template be allowed to easily break the transform? I say No.

In the docs at https://pypi.org/project/plone.outputfilters/ it says

The captioned version of an image is rendered using the @@plone.outputfilters_captioned_image view, which may be overridden to customize the caption.

captioned_image.pt https://github.com/plone/plone.outputfilters/blob/7b2c5b1b4697c4d45b7eb2a01a07ca4666e30afa/plone/outputfilters/browser/captioned_image.pt

By neglecting the <a> tag in the customized captioned_image.pt template, #39 gets aggravated.

This can be avoided by not assuming the captioned image template has an <a> tag. Current code assumes it does, and forces all custom captioned image templates to also have one.

In this code: https://github.com/plone/plone.outputfilters/blob/7b2c5b1b4697c4d45b7eb2a01a07ca4666e30afa/plone/outputfilters/filters/resolveuid_and_caption.py#L348

        captioned = BeautifulSoup(
            self.captioned_image_template(**options), 'html.parser')

        # if we are a captioned image within a link, remove and occurrences
        # of a tags inside caption template to preserve the outer link
        if bool(elem.find_parent('a')):
            captioned.a.unwrap()

AttributeError: 'NoneType' object has no attribute 'unwrap' Because 'captioned.a' is None

I think we should check if 'captioned.a' even exists before attempting to remove it. This allows for more flexibility in user overridden templates.

Bottom line - changing around some harmless html in a template - that is even documented how to override - should not break the transform and throw a 500.

Thoughts?

flipmcf commented 4 years ago

Closing this. #39 is really and truly a bug in plone.