[#6431] changing "Simple Example" to use implode/explode #6581

mccullagh · 2016-05-21T10:11:05Z

Q	A
Doc fix?	yes
New docs?	no
Applies to	all
Fixed tickets	#6431

mccullagh · 2016-05-21T10:12:47Z

I preferred to use labelsand not tags.

HeahDude · 2016-05-21T10:13:14Z

cookbook/form/data_transformers.rst

@@ -16,24 +16,24 @@ to render the form, and then back into a ``DateTime`` object on submit.
    When a form field has the ``inherit_data`` option set, Data Transformers
    won't be applied to that field.

-Simple Example: Sanitizing HTML on User Input
+Simple Example: transforming labels string from User Input to array


Should be Transforming Labels String from User Input to Array

HeahDude · 2016-05-21T10:14:34Z

cookbook/form/data_transformers.rst


    // ...
    class TaskType extends AbstractType
    {
        public function buildForm(FormBuilderInterface $builder, array $options)
        {
-            $builder->add('description', TextareaType::class);
+            $builder->add('labels', TextType::class);


Shouldn't this go on 2.3?

javiereguiluz · 2016-05-21T10:15:26Z

cookbook/form/data_transformers.rst

-
-#. Your users are allowed to use *some* HTML tags, but not others: you need a way
-   to call :phpfunction:`strip_tags` after the form is submitted;
+Internally we want to handle the ``labels`` as array, but to have the form simple we wanna allow the User to edit them as a string.


We don't use "we" in the docs. What if we reword this as:

Internally the labels are stored as an array, but they are displayed to the user as a simple string, to make them easier to edit.

HeahDude · 2016-05-21T10:16:59Z

tags field is used elsewhere in the docs, we should keep it for consistency. What's the advantage of using labels?

javiereguiluz · 2016-05-21T10:17:02Z

cookbook/form/data_transformers.rst

-                    function ($originalDescription) {
-                        return preg_replace('#<br\s*/?>#i', "\n", $originalDescription);
+                    // transform array to string so the input reads easier
+                    function ($originalLabels) {


What if we rename these variables:

$originalLabels -> $labelsAsArray $submittedLabels -> $labelsAsString

This is just a proposal. Please don't make any change until our doc managers approve/reject this idea. Thanks!

I like your proposal

original/submitted keeps an "event" context, and I like that.

original/submitted helps to identify the source of the Callbacks.

wouterj · 2016-05-21T10:19:59Z

Looks great! I don't care much about labels vs tags. After you applied the open comments, I think it's ready to be merged. Thanks for working on this!

javiereguiluz · 2016-05-21T11:01:04Z

@wouterj I think consistency is important here. As @HeahDude said, we use tags everywhere ... so I'm 👍 to replace labels by tags.

HeahDude · 2016-05-21T11:04:32Z

cookbook/form/data_transformers.rst


    $builder->add(
-        $builder->create('description', TextareaType::class)
+        $builder->create('description', TextType::class)


Since text type is the default, I would suggest to remove the second argument everywhere to easy the merge in all branches.

Why not also add a tip to handle null as an empty string, we could then remove the hint in master and make a PR to document it in empty_data option for the text type (ref symfony/symfony#18357)?

I've pushed 133a77c, this should really go on 2.3 imo.

removing the TextTypemeans i need to remove the data_classin the example too because else it would not be clear anymore which Type it is.

You can still pass null

xabbuh · 2016-05-21T11:43:16Z

cookbook/form/data_transformers.rst


    // ...
    class TaskType extends AbstractType
    {
        public function buildForm(FormBuilderInterface $builder, array $options)
        {
-            $builder->add('description', TextareaType::class);
+            $builder->add('tags', TextType::class);


I agree that we should merge this into the 2.3 branch first. You don't need to create a new PR for this (we can rebase when merging), but can you please change the type here to be simply text (we will then update the type when merging things up)?

xabbuh · 2016-05-21T11:45:16Z

cookbook/form/data_transformers.rst

@@ -16,24 +16,24 @@ to render the form, and then back into a ``DateTime`` object on submit.
    When a form field has the ``inherit_data`` option set, Data Transformers
    won't be applied to that field.

-Simple Example: Sanitizing HTML on User Input
---------------------------------------------


Can you please add a label to be BC with the old headline? This should look like this:

.. _simple-example-sanitizing-html-on-user-input:

wouterj · 2016-05-21T11:53:29Z

👍

If you don't have much time, we can fix @xabbuh's comments during the merge as well.

mccullagh · 2016-05-21T12:02:57Z

I have not added a label to be BC with the old headline. I'm not sure how it should be in reStructuredText.

HeahDude · 2016-05-21T12:06:17Z

cookbook/form/data_transformers.rst

-                    function ($originalDescription) {
-                        return preg_replace('#<br\s*/?>#i', "\n", $originalDescription);
+                    // transform array to string so the input reads easier
+                      function ($originalTags) {


You should revert the extra spaces here

wouterj · 2016-05-21T12:12:53Z

@mccullagh you do that by adding the line written by @xabbuh before the new headline. So you end up with this:

.. _simple-example-sanitizing-html-on-user-input:

Simple Example: Transforming String Tags from User Input to an Array
--------------------------------------------------------------------

wouterj · 2016-05-21T13:42:41Z

Looks perfect, 👍 !

xabbuh · 2016-05-21T13:45:50Z

👍

…(mccullagh) This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes #6581). Discussion ---------- [#6431] changing "Simple Example" to use implode/explode | Q | A | ------------- | --- | Doc fix? | yes | New docs? | no | Applies to | all | Fixed tickets | #6431 Commits ------- 4b8a755 [#6431] changing "Simple Example" to use implode/explode

wouterj · 2016-05-21T14:21:38Z

Thanks @mccullagh for writing this! I've merged this into the 2.3 branch via d042351 and made some minor improvements afterwards in c8f4583 . I'll now merge them into the newer branches and update the form type names.

mccullagh · 2016-05-21T14:24:59Z

Thank you @wouterj and everyone who helped.

HeahDude reviewed May 21, 2016
View reviewed changes

[symfony#6431] changing "Simple Example" to use implode/explode

495ca23

HeahDude reviewed May 21, 2016
View reviewed changes

mccullagh force-pushed the feature/data_transforming_example branch from cad6fce to 495ca23 Compare May 21, 2016 10:14

javiereguiluz reviewed May 21, 2016
View reviewed changes

wouterj added the Status: Needs Work label May 21, 2016

mccullagh changed the title ~~[#6431] changing "Simple Example" to use implode/explode~~ [WIP] [#6431] changing "Simple Example" to use implode/explode May 21, 2016

mccullagh added 2 commits May 21, 2016 12:40

fix missing space after the comma

45add8b

changed explanation text

83f689c

HeahDude reviewed May 21, 2016
View reviewed changes

Example from Labels to Tags

15f053b

xabbuh reviewed May 21, 2016
View reviewed changes

wouterj added Status: Reviewed and removed Status: Needs Work labels May 21, 2016

xabbuh reviewed May 21, 2016
View reviewed changes

change simply text

216b228

HeahDude reviewed May 21, 2016
View reviewed changes

mccullagh added 2 commits May 21, 2016 14:22

add a label to be BC with the old headline

310f6a4

revert extra spaces

9dcc4b6

mccullagh changed the title ~~[WIP] [#6431] changing "Simple Example" to use implode/explode~~ [#6431] changing "Simple Example" to use implode/explode May 21, 2016

wouterj added a commit that referenced this pull request May 21, 2016

[#6581] Small improvements

2333190

wouterj added a commit that referenced this pull request May 21, 2016

[#6581] Some minor improvements

c8f4583

wouterj closed this May 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#6431] changing "Simple Example" to use implode/explode #6581

[#6431] changing "Simple Example" to use implode/explode #6581

mccullagh commented May 21, 2016

mccullagh commented May 21, 2016

HeahDude May 21, 2016

HeahDude May 21, 2016

javiereguiluz May 21, 2016

HeahDude commented May 21, 2016

javiereguiluz May 21, 2016

wouterj May 21, 2016

HeahDude May 21, 2016

mccullagh May 21, 2016

wouterj commented May 21, 2016

javiereguiluz commented May 21, 2016

HeahDude May 21, 2016

HeahDude May 21, 2016 •

edited

Loading

mccullagh May 21, 2016

HeahDude May 21, 2016

xabbuh May 21, 2016

xabbuh May 21, 2016

wouterj commented May 21, 2016

mccullagh commented May 21, 2016

HeahDude May 21, 2016

wouterj commented May 21, 2016 •

edited

Loading

wouterj commented May 21, 2016

xabbuh commented May 21, 2016

wouterj commented May 21, 2016

mccullagh commented May 21, 2016

[#6431] changing "Simple Example" to use implode/explode #6581

[#6431] changing "Simple Example" to use implode/explode #6581

Conversation

mccullagh commented May 21, 2016

mccullagh commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented May 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented May 21, 2016

javiereguiluz commented May 21, 2016

Choose a reason for hiding this comment

HeahDude May 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented May 21, 2016

mccullagh commented May 21, 2016

Choose a reason for hiding this comment

wouterj commented May 21, 2016 • edited Loading

wouterj commented May 21, 2016

xabbuh commented May 21, 2016

wouterj commented May 21, 2016

mccullagh commented May 21, 2016

HeahDude May 21, 2016 •

edited

Loading

wouterj commented May 21, 2016 •

edited

Loading