Movatterモバイル変換

		@@ -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 onUser Input
		Simple Example:transforming labels string fromUser Input to array

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Should beTransforming Labels String from User Input to Array

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

495ca23

HeahDude reviewed


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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Shouldn't this go on 2.3?

mccullagh force-pushed thefeature/data_transforming_example branch fromcad6fce to495ca23Compare

May 21, 2016 10:14

javiereguiluz reviewed


		#. 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.

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 displayedto the user as a simple string, to make them easier to edit.

Copy link

Contributor

HeahDude commentedMay 21, 2016

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

javiereguiluz reviewed

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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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!

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I like your proposal

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

wouterj added the Status: Needs Work label

Copy link

Member

wouterj commentedMay 21, 2016

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!

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

mccullagh added2 commits

May 21, 2016 12:40

fix missing space after the comma

45add8b

changed explanation text

83f689c

Copy link

Member

javiereguiluz commentedMay 21, 2016

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

HeahDude reviewed


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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 handlenull as an empty string, we could then remove the hint in master and make a PR to document it inempty_data option for the text type (refsymfony/symfony#18357)?

Copy link

Contributor

HeahDudeMay 21, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I've pushed133a77c, this should really go on 2.3 imo.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You can still passnull

Example from Labels to Tags

15f053b

xabbuh reviewed


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

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I agree that we should merge this into the2.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 simplytext (we will then update the type when merging things up)?

wouterj added Status: Reviewed and removed Status: Needs Work labels

xabbuh reviewed

		@@ -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
		---------------------------------------------

Copy link

Member

xabbuhMay 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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:

Copy link

Member

wouterj commentedMay 21, 2016

👍

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

change simply text

216b228

Copy link

ContributorAuthor

mccullagh commentedMay 21, 2016

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

HeahDude reviewed

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

Copy link

Contributor

HeahDudeMay 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

You should revert the extra spaces here

Copy link

Member

wouterj commentedMay 21, 2016•
edited
Loading

@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--------------------------------------------------------------------

mccullagh added2 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

Copy link

Member

wouterj commentedMay 21, 2016

Looks perfect, 👍 !

Copy link

Member

xabbuh commentedMay 21, 2016

👍

wouterj added a commit that referenced this pull request

[#6581] Small improvements

2333190

wouterj added a commit that referenced this pull request

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

d042351

…(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 |#6431Commits-------4b8a755 [#6431] changing "Simple Example" to use implode/explode

wouterj added a commit that referenced this pull request

[#6581] Some minor improvements

c8f4583

Copy link

Member

wouterj commentedMay 21, 2016

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

wouterj closed this