symfony/symfony-docsPublic

NotificationsYou must be signed in to change notification settings
Fork5.3k
Star2.3k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

mccullagh wants to merge7 commits intosymfony:masterfrommccullagh:feature/data_transforming_example

Closed

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

mccullagh wants to merge7 commits intosymfony:masterfrommccullagh:feature/data_transforming_example

Conversation

Copy link

Contributor

mccullagh commentedMay 21, 2016

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

Copy link

ContributorAuthor

mccullagh commentedMay 21, 2016

I preferred to uselabelsand nottags.

HeahDude reviewed

May 21, 2016

View reviewed changes

cookbook/form/data_transformers.rst Outdated

		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

May 21, 2016

View reviewed changes

cookbook/form/data_transformers.rst Outdated

		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

May 21, 2016

View reviewed changes

cookbook/form/data_transformers.rst Outdated


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

May 21, 2016

View reviewed changes

cookbook/form/data_transformers.rst Outdated

		function ($originalDescription) {
		returnpreg_replace('#<br\s*/?>#i',"\n", $originalDescription);
		// transformarray tostring so theinput 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

May 21, 2016

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

May 21, 2016

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

May 21, 2016

View reviewed changes

cookbook/form/data_transformers.rst Outdated


		$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

May 21, 2016

View reviewed changes

cookbook/form/data_transformers.rst Outdated

		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

May 21, 2016

xabbuh reviewed

May 21, 2016

View reviewed changes

cookbook/form/data_transformers.rst

		won't be applied to that field.

		Simple Example:Sanitizing HTML onUser 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

May 21, 2016

View reviewed changes

cookbook/form/data_transformers.rst Outdated

		function ($originalDescription) {
		returnpreg_replace('#<br\s*/?>#i', "\n", $originalDescription);
		// transformarray tostring so theinput 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

May 21, 2016

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

May 21, 2016

[#6581] Small improvements

2333190

wouterj added a commit that referenced this pull request

May 21, 2016

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

May 21, 2016

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

May 21, 2016

Copy link

ContributorAuthor

mccullagh commentedMay 21, 2016

Thank you@wouterj and everyone who helped.

Labels

Status: Reviewed

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

mccullagh commentedMay 21, 2016

Uh oh!

mccullagh commentedMay 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDude commentedMay 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commentedMay 21, 2016

Uh oh!

javiereguiluz commentedMay 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeahDudeMay 21, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commentedMay 21, 2016

Uh oh!

mccullagh commentedMay 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commentedMay 21, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

wouterj commentedMay 21, 2016

Uh oh!

xabbuh commentedMay 21, 2016

Uh oh!

wouterj commentedMay 21, 2016

Uh oh!

mccullagh commentedMay 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HeahDudeMay 21, 2016•
edited
Loading

wouterj commentedMay 21, 2016•
edited
Loading