Avoid calling to_python when retrieving form field values in FieldBlock.

to_python and get_prep_value should only be used for serialising / deserialising between native block values and their JSONish representations. Instead, we add two new methods value_for_form and value_from_form to handle any necessary conversions between form values and block values.

wagtail/wagtailcore/blocks/field_block.py

@@ -17,6 +17,7 @@ from .base import Block
 
 
 class FieldBlock(Block):
+    """A block that wraps a Django form field"""
     class Meta:
         default = None
 
@@ -33,11 +34,13 @@ class FieldBlock(Block):
 
         widget_attrs = {'id': prefix, 'placeholder': self.label}
 
+        field_value = self.value_for_form(value)
+
         if hasattr(widget, 'render_with_errors'):
-            widget_html = widget.render_with_errors(prefix, value, attrs=widget_attrs, errors=errors)
+            widget_html = widget.render_with_errors(prefix, field_value, attrs=widget_attrs, errors=errors)
             widget_has_rendered_errors = True
         else:
-            widget_html = widget.render(prefix, value, attrs=widget_attrs)
+            widget_html = widget.render(prefix, field_value, attrs=widget_attrs)
             widget_has_rendered_errors = False
 
         return render_to_string('wagtailadmin/block_forms/field.html', {
@@ -50,11 +53,34 @@ class FieldBlock(Block):
             'errors': errors if (not widget_has_rendered_errors) else None
         })
 
+    def value_from_form(self, value):
+        """
+        The value that we get back from the form field might not be the type
+        that this block works with natively; for example, the block may want to
+        wrap a simple value such as a string in an object that provides a fancy
+        HTML rendering (e.g. EmbedBlock).
+
+        We therefore provide this method to perform any necessary conversion
+        from the form field value to the block's native value. As standard,
+        this returns the form field value unchanged.
+        """
+        return value
+
+    def value_for_form(self, value):
+        """
+        Reverse of value_from_form; convert a value of this block's native value type
+        to one that can be rendered by the form field
+        """
+        return value
+
     def value_from_datadict(self, data, files, prefix):
-        return self.to_python(self.field.widget.value_from_datadict(data, files, prefix))
+        return self.value_from_form(self.field.widget.value_from_datadict(data, files, prefix))
 
     def clean(self, value):
-        return self.field.clean(value)
+        # We need an annoying value_for_form -> value_from_form round trip here to account for
+        # the possibility that the form field is set up to validate a different value type to
+        # the one this block works with natively
+        return self.value_from_form(self.field.clean(self.value_for_form(value)))
 
 
 class CharBlock(FieldBlock):
@@ -248,7 +274,8 @@ class ChooserBlock(FieldBlock):
         return forms.ModelChoiceField(queryset=self.target_model.objects.all(), widget=self.widget, required=self.required)
 
     def to_python(self, value):
-        if value is None or isinstance(value, self.target_model):
+        # the incoming serialised value should be None or an ID
+        if value is None:
             return value
         else:
             try:
@@ -257,10 +284,21 @@ class ChooserBlock(FieldBlock):
                 return None
 
     def get_prep_value(self, value):
-        if isinstance(value, self.target_model):
-            return value.id
+        # the native value (a model instance or None) should serialise to an ID or None
+        if value is None:
+            return None
         else:
+            return value.id
+
+    def value_from_form(self, value):
+        # ModelChoiceField sometimes returns an ID, and sometimes an instance; we want the instance
+        if value is None or isinstance(value, self.target_model):
             return value
+        else:
+            try:
+                return self.target_model.objects.get(pk=value)
+            except self.target_model.DoesNotExist:
+                return None
 
     def clean(self, value):
         # ChooserBlock works natively with model instances as its 'value' type (because that's what you
@@ -274,6 +312,7 @@ class ChooserBlock(FieldBlock):
             value = value.pk
         return super(ChooserBlock, self).clean(value)
 
+
 class PageChooserBlock(ChooserBlock):
     @cached_property
     def target_model(self):

wagtail/wagtailcore/tests/test_blocks.py

@@ -3,8 +3,12 @@ import unittest
 from django import forms
 from django.forms.utils import ErrorList
 from django.core.exceptions import ValidationError
+from django.test import TestCase
 
 from wagtail.wagtailcore import blocks
+from wagtail.wagtailcore.models import Page
+
+import base64
 
 
 class TestFieldBlock(unittest.TestCase):
@@ -81,6 +85,26 @@ class TestFieldBlock(unittest.TestCase):
 
         self.assertEqual(content, ["Choice 1"])
 
+    def test_form_handling_is_independent_of_serialisation(self):
+        class Base64EncodingCharBlock(blocks.CharBlock):
+            """A CharBlock with a deliberately perverse JSON (de)serialisation format
+            so that it visibly blows up if we call to_python / get_prep_value where we shouldn't"""
+
+            def to_python(self, jsonish_value):
+                # decode as base64 on the way out of the JSON serialisation
+                return base64.b64decode(jsonish_value)
+
+            def get_prep_value(self, native_value):
+                # encode as base64 on the way into the JSON serialisation
+                return base64.b64encode(native_value)
+
+        block = Base64EncodingCharBlock()
+        form_html = block.render_form('hello world', 'title')
+        self.assertIn('value="hello world"', form_html)
+
+        value_from_form = block.value_from_datadict({'title': 'hello world'}, {}, 'title')
+        self.assertEqual('hello world', value_from_form)
+
 
 class TestChoiceBlock(unittest.TestCase):
     def setUp(self):
@@ -1019,3 +1043,60 @@ class TestStreamBlock(unittest.TestCase):
         self.assertEqual(len(stream_value), 1)
         self.assertEqual(stream_value[0].block_type, 'heading')
         self.assertEqual(stream_value[0].value, 'A different default heading')
+
+
+class TestPageChooserBlock(TestCase):
+    fixtures = ['test.json']
+
+    def test_serialize(self):
+        """The value of a PageChooserBlock (a Page object) should serialize to an ID"""
+        block = blocks.PageChooserBlock()
+        christmas_page = Page.objects.get(slug='christmas')
+
+        self.assertEqual(block.get_prep_value(christmas_page), christmas_page.id)
+
+        # None should serialize to None
+        self.assertEqual(block.get_prep_value(None), None)
+
+    def test_deserialize(self):
+        """The serialized value of a PageChooserBlock (an ID) should deserialize to a Page object"""
+        block = blocks.PageChooserBlock()
+        christmas_page = Page.objects.get(slug='christmas')
+
+        self.assertEqual(block.to_python(christmas_page.id), christmas_page)
+
+        # None should deserialize to None
+        self.assertEqual(block.to_python(None), None)
+
+    def test_form_render(self):
+        block = blocks.PageChooserBlock()
+
+        empty_form_html = block.render_form(None, 'page')
+        self.assertIn('<input id="page" name="page" placeholder="" type="hidden" />', empty_form_html)
+
+        christmas_page = Page.objects.get(slug='christmas')
+        christmas_form_html = block.render_form(christmas_page, 'page')
+        expected_html = '<input id="page" name="page" placeholder="" type="hidden" value="%d" />' % christmas_page.id
+        self.assertIn(expected_html, christmas_form_html)
+
+    def test_form_response(self):
+        block = blocks.PageChooserBlock()
+        christmas_page = Page.objects.get(slug='christmas')
+
+        value = block.value_from_datadict({'page': str(christmas_page.id)}, {}, 'page')
+        self.assertEqual(value, christmas_page)
+
+        empty_value = block.value_from_datadict({'page': ''}, {}, 'page')
+        self.assertEqual(empty_value, None)
+
+    def test_clean(self):
+        required_block = blocks.PageChooserBlock()
+        nonrequired_block = blocks.PageChooserBlock(required=False)
+        christmas_page = Page.objects.get(slug='christmas')
+
+        self.assertEqual(required_block.clean(christmas_page), christmas_page)
+        with self.assertRaises(ValidationError):
+            required_block.clean(None)
+
+        self.assertEqual(nonrequired_block.clean(christmas_page), christmas_page)
+        self.assertEqual(nonrequired_block.clean(None), None)