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

Fix non-callable accessor when accessor is not defined #94

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@zemm
Contributor

zemm commented Jul 5, 2017

I was looking at a broken RichTextField on Products.PloneFormGen (smcmahon/Products.PloneFormGen#173)
and the (one of the) problem(s) seemed to be a field with an accessor that was None.
Instead of fixing the specific case, I looked at Archetype's
implemention of Field::getAccessor(), and noticed that

a) It's documented as "Return the accessor method ..."
b) If accessor is not set, it defaults to None - which is not
a method as advertised
c) getAccessor() is used only via happy-path everywhere at
Products.Archetypes1 like so:field.getAccessor(context)()`

I thought the most propriate fix would be to make the fallback
safe by returning a function evaluating to None.

Waiting for input

I'm not familiar with Plone, so I'll wait an input on whether something could potentially depend on this seemingly broken implementation, and wether this should be marked as a bugfix or breaking change, if accepted at all?

zemm added some commits Jul 5, 2017

Fix non-callable accessor when accessor is not defined
I was looking at a broken RichTextField on Products.PloneFormGen
and the problem seemed to be a field with an unset accessor.
Instead of fixing the specific case, I looked at Archetype's
implemention of Field::getAccessor(), and noticed that

a) It's documented as "Return the accessor method"
b) If accessor is not set, it defaults to None - which is not
   a method as advertised
c) getAccessor() is used only via happy-path everywhere at
   Products.Archetypes like so: field.getAccessor(context)()

I thought the most propriate fix would be to make the fallback
safe by returning a function evaluating to None.
@mauritsvanrees

This comment has been minimized.

Show comment
Hide comment
@mauritsvanrees

mauritsvanrees Jul 8, 2017

Member

Archetypes is old. If this was really a problem, then I would have expected this to pop up sooner. That is no guarantee though.

There is code in Archetypes that knows that getAccessor could return None and checks for it. With the current code, this validation snippet would continue to the next field. With your code, the snippet would set value = None and this would end up in the field data and may cause a validation error.

There should be a difference between 'this field has no accessor' and 'the value of this field is None'. With your fix, this difference would go away, which is probably not good.

The Travis tests passed though. I have started the Jenkins tests, which runs all core Plone tests. Let's see what that says.
Regardless of the result, I think this is a problem specific for PloneFormGen and should be fixed there.

Member

mauritsvanrees commented Jul 8, 2017

Archetypes is old. If this was really a problem, then I would have expected this to pop up sooner. That is no guarantee though.

There is code in Archetypes that knows that getAccessor could return None and checks for it. With the current code, this validation snippet would continue to the next field. With your code, the snippet would set value = None and this would end up in the field data and may cause a validation error.

There should be a difference between 'this field has no accessor' and 'the value of this field is None'. With your fix, this difference would go away, which is probably not good.

The Travis tests passed though. I have started the Jenkins tests, which runs all core Plone tests. Let's see what that says.
Regardless of the result, I think this is a problem specific for PloneFormGen and should be fixed there.

@mauritsvanrees

This comment has been minimized.

Show comment
Hide comment
@mauritsvanrees

mauritsvanrees Jul 8, 2017

Member

Well, the Jenkins job actually passes, so that is good, but I still don't think this is the real solution. I guess this code path is not well tested.

Let me post a sample traceback first:

ERROR Zope.SiteErrorLog 1499536698.10.339036551188 
http://127.0.0.1:8080/Plone5nl/form/opgemaakt-tekst-veld/fg_base_view_p3
Traceback (innermost last):
  Module ZPublisher.Publish, line 138, in publish
  Module ZPublisher.mapply, line 77, in mapply
  Module ZPublisher.Publish, line 48, in call_object
  Module Products.CMFFormController.FSControllerPageTemplate, line 91, in __call__
  Module Products.CMFFormController.BaseControllerPageTemplate, line 32, in _call
  Module Shared.DC.Scripts.Bindings, line 322, in __call__
  Module Shared.DC.Scripts.Bindings, line 359, in _bindAndExec
  Module Products.CMFCore.FSPageTemplate, line 237, in _exec
  Module Products.CMFCore.FSPageTemplate, line 177, in pt_render
  Module Products.PageTemplates.PageTemplate, line 87, in pt_render
  Module zope.pagetemplate.pagetemplate, line 132, in pt_render
  Module five.pt.engine, line 98, in __call__
  Module z3c.pt.pagetemplate, line 163, in render
  Module chameleon.zpt.template, line 261, in render
  Module chameleon.template, line 191, in render
  Module chameleon.template, line 171, in render
  Module 4dbe49b1edf39224c3ead3250ff24d8e.py, line 977, in render
  Module e6ffeb0d058b24dc2d5126c89b16b68d.py, line 1223, in render_master
  Module e6ffeb0d058b24dc2d5126c89b16b68d.py, line 420, in render_content
  Module 4dbe49b1edf39224c3ead3250ff24d8e.py, line 965, in __fill_content_core
  Module 4dbe49b1edf39224c3ead3250ff24d8e.py, line 332, in render_content_core
  Module 7290882a3abc26dd81b7aab7804882a4.py, line 368, in render_body
  Module a979a04985ccf908e1b409af80602a91.py, line 138, in render_edit
  Module 02b341a817626f89511af74d637f9005.py, line 665, in render_edit
  Module a979a04985ccf908e1b409af80602a91.py, line 100, in __fill_widget_body
  Module Products.Archetypes.Widget, line 1425, in edit
  Module Products.Archetypes.Widget, line 1395, in _base_args
TypeError: 'NoneType' object is not callable

It's not clear where this could be fixed in PloneFormGen, as my first suggestion was. Maybe an accessor could be defined on PloneFormGen fields.

But with your fix it would simply go wrong differently. See these lines. Simplified, it calls self.request.get(name, accessor()).decode('utf-8'). So if the call to accessor returns None, then you get None.decode('utf-8'), which gives an AttributeError.

My suggestion would be to fix the _base_args method of the TinyMCEWidget to use an empty string when the accessor is None (or when the accessor returns None).
Possibly similar fixes would need to be done in other parts of Archetypes code, but this one would solve the current problem.
Could you try this?

Member

mauritsvanrees commented Jul 8, 2017

Well, the Jenkins job actually passes, so that is good, but I still don't think this is the real solution. I guess this code path is not well tested.

Let me post a sample traceback first:

ERROR Zope.SiteErrorLog 1499536698.10.339036551188 
http://127.0.0.1:8080/Plone5nl/form/opgemaakt-tekst-veld/fg_base_view_p3
Traceback (innermost last):
  Module ZPublisher.Publish, line 138, in publish
  Module ZPublisher.mapply, line 77, in mapply
  Module ZPublisher.Publish, line 48, in call_object
  Module Products.CMFFormController.FSControllerPageTemplate, line 91, in __call__
  Module Products.CMFFormController.BaseControllerPageTemplate, line 32, in _call
  Module Shared.DC.Scripts.Bindings, line 322, in __call__
  Module Shared.DC.Scripts.Bindings, line 359, in _bindAndExec
  Module Products.CMFCore.FSPageTemplate, line 237, in _exec
  Module Products.CMFCore.FSPageTemplate, line 177, in pt_render
  Module Products.PageTemplates.PageTemplate, line 87, in pt_render
  Module zope.pagetemplate.pagetemplate, line 132, in pt_render
  Module five.pt.engine, line 98, in __call__
  Module z3c.pt.pagetemplate, line 163, in render
  Module chameleon.zpt.template, line 261, in render
  Module chameleon.template, line 191, in render
  Module chameleon.template, line 171, in render
  Module 4dbe49b1edf39224c3ead3250ff24d8e.py, line 977, in render
  Module e6ffeb0d058b24dc2d5126c89b16b68d.py, line 1223, in render_master
  Module e6ffeb0d058b24dc2d5126c89b16b68d.py, line 420, in render_content
  Module 4dbe49b1edf39224c3ead3250ff24d8e.py, line 965, in __fill_content_core
  Module 4dbe49b1edf39224c3ead3250ff24d8e.py, line 332, in render_content_core
  Module 7290882a3abc26dd81b7aab7804882a4.py, line 368, in render_body
  Module a979a04985ccf908e1b409af80602a91.py, line 138, in render_edit
  Module 02b341a817626f89511af74d637f9005.py, line 665, in render_edit
  Module a979a04985ccf908e1b409af80602a91.py, line 100, in __fill_widget_body
  Module Products.Archetypes.Widget, line 1425, in edit
  Module Products.Archetypes.Widget, line 1395, in _base_args
TypeError: 'NoneType' object is not callable

It's not clear where this could be fixed in PloneFormGen, as my first suggestion was. Maybe an accessor could be defined on PloneFormGen fields.

But with your fix it would simply go wrong differently. See these lines. Simplified, it calls self.request.get(name, accessor()).decode('utf-8'). So if the call to accessor returns None, then you get None.decode('utf-8'), which gives an AttributeError.

My suggestion would be to fix the _base_args method of the TinyMCEWidget to use an empty string when the accessor is None (or when the accessor returns None).
Possibly similar fixes would need to be done in other parts of Archetypes code, but this one would solve the current problem.
Could you try this?

@datakurre

This comment has been minimized.

Show comment
Hide comment
@datakurre

datakurre Jul 9, 2017

Member

@mauritsvanrees Thanks for you review. I recall that @zemm did initially think about fixing this just in TinyMCEWidget, but then started wondering if this is more general issue and more proper fix is needed.

I agree that because we cannot be sure if that codepath is tested enough and if someone is already depending on the current behavior, fixing it for TinyMCEWidget only is good enough.

Member

datakurre commented Jul 9, 2017

@mauritsvanrees Thanks for you review. I recall that @zemm did initially think about fixing this just in TinyMCEWidget, but then started wondering if this is more general issue and more proper fix is needed.

I agree that because we cannot be sure if that codepath is tested enough and if someone is already depending on the current behavior, fixing it for TinyMCEWidget only is good enough.

@zemm

This comment has been minimized.

Show comment
Hide comment
@zemm

zemm Jul 25, 2017

Contributor

It was my assumption that such a change might not be feasible in such an old package. Would it be sensible to to update the method doc for future users to better reflect the reality, something like:

Return the accessor method for getting data out of this field, or None if the field has no accessor?

Contributor

zemm commented Jul 25, 2017

It was my assumption that such a change might not be feasible in such an old package. Would it be sensible to to update the method doc for future users to better reflect the reality, something like:

Return the accessor method for getting data out of this field, or None if the field has no accessor?

@tkimnguyen

This comment has been minimized.

Show comment
Hide comment
@tkimnguyen

tkimnguyen Jul 29, 2017

Member

:( I was hoping one of you would suggest the fix in PFG :)

Member

tkimnguyen commented Jul 29, 2017

:( I was hoping one of you would suggest the fix in PFG :)

@datakurre

This comment has been minimized.

Show comment
Hide comment
@datakurre

datakurre Jul 29, 2017

Member

AFAIK it could be fixed in PFG by customising the widget (using inheritance) in PFG. Good point that it might be the easiest option to merge.

Member

datakurre commented Jul 29, 2017

AFAIK it could be fixed in PFG by customising the widget (using inheritance) in PFG. Good point that it might be the easiest option to merge.

@jensens

This comment has been minimized.

Show comment
Hide comment
@jensens

jensens Aug 1, 2017

Member

I am ok with the change in documentation. I don't think we should change Archetypes, even if this is probably a valid issue.

Member

jensens commented Aug 1, 2017

I am ok with the change in documentation. I don't think we should change Archetypes, even if this is probably a valid issue.

@tkimnguyen

This comment has been minimized.

Show comment
Hide comment
@tkimnguyen

tkimnguyen Aug 22, 2017

Member

@zemm it sounds like you need to change the doc only. Does someone wants to make the fix in PFG (too or instead)?

Member

tkimnguyen commented Aug 22, 2017

@zemm it sounds like you need to change the doc only. Does someone wants to make the fix in PFG (too or instead)?

@zemm

This comment has been minimized.

Show comment
Hide comment
@zemm

zemm Aug 29, 2017

Contributor

Closing in favor of the doc change #96

Contributor

zemm commented Aug 29, 2017

Closing in favor of the doc change #96

@zemm zemm closed this Aug 29, 2017

@zemm zemm deleted the fix-non-callable-accessor branch Aug 29, 2017

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