Skip to content

[Form] moved data trimming logic of TrimListener into StringUtil #14146

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Form] moved data trimming logic of TrimListener into StringUtil #14146

wants to merge 1 commit into from

Conversation

issei-m
Copy link
Contributor

@issei-m issei-m commented Mar 31, 2015

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

In the small job which needs not to build a form (e.g. just process a value gotten from Request), I want to reuse the trim logic.

@issei-m issei-m mentioned this pull request Mar 31, 2015
Closed
@dosten
Copy link
Contributor

dosten commented Apr 1, 2015

Why put into StringUtil and not in FormUtil? Also, the @author of the StringUtil class must be you.

@issei-m
Copy link
Contributor Author

issei-m commented Apr 1, 2015

@dosten I also thought so but I decided to put into a new class because FormUtil is singular form. Should this code be moved into FormUtil?

the @author of the StringUtil class must be you.

TrimListener was introduced by @webmozart at first. So it would be better to be his name.

Tobion
Tobion reviewed Apr 1, 2015
View reviewed changes
src/Symfony/Component/Form/Util/StringUtil.php
/**
* Returns the trimmed data.
*
* @param mixed $data
Copy link
Contributor

Choose a reason for hiding this comment

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

it should only accept 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.

well, this would force to keep the is_string check in TrimListener though

Copy link
Contributor

Choose a reason for hiding this comment

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

which is way better in terms of single responsibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

why would this method handle non-strings here. doesn't make sense. it's not even documented that non-strings are returned as-is.

Copy link
Contributor

Choose a reason for hiding this comment

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

also this will allow us to add scalar type hints later for php 7.

@issei-m issei-m force-pushed the form-string-util branch from 5bfeec3 to f222c6d Compare April 2, 2015 13:39
@issei-m
Copy link
Contributor Author

issei-m commented Apr 2, 2015

I've just updated PR. Now, StringUtil does not concern argument type.
BTW, it would be better to add my name to @author in a patch like this? (I just moved codes, though)

@issei-m
Copy link
Contributor Author

issei-m commented Apr 14, 2015

ping @stof @Tobion

Tobion
Tobion reviewed Apr 14, 2015
View reviewed changes
src/Symfony/Component/Form/Tests/Util/StringUtilTest.php
use Symfony\Component\Form\Util\StringUtil;

/**
* @author Bernhard Schussek <bschussek@gmail.com>
Copy link
Contributor

Choose a reason for hiding this comment

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

test classes dont have this annotation

@issei-m
Copy link
Contributor Author

issei-m commented Apr 14, 2015

@Tobion fixed it.

And how about this?

BTW, it would be better to add my name to @author in a patch like this? (I just moved codes, though)

@xabbuh
Copy link
Member

xabbuh commented Apr 14, 2015

@issei-m As you just moved code around, keeping @webmozart as the author is the best.

@issei-m
Copy link
Contributor Author

issei-m commented Apr 14, 2015

@xabbuh Thanks.

Well then, all issues were resolved.
Ready to be merged?

@webmozart
Copy link
Contributor

About the authorship, you should include yourself as author as well. I'd be pretty confused if I'd find myself as only author of a class that I had never seen before ;)

I think this is fine, but it can't be merged into 2.7 as that branch is feature frozen.

@issei-m
Copy link
Contributor Author

issei-m commented May 16, 2015

@webmozart Thanks for your advice. I'll try to open a new PR soon.

@issei-m issei-m closed this May 16, 2015
@issei-m issei-m mentioned this pull request May 16, 2015
Closed
webmozart added a commit that referenced this pull request Jun 22, 2015
@webmozart
feature #14660 [Form] moved data trimming logic of TrimListener into …
3151614 
…StringUtil (issei-m)

This PR was squashed before being merged into the 2.8 branch (closes #14660).

Discussion
----------

[Form] moved data trimming logic of TrimListener into StringUtil

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

I introduced this PR at #14146, but I didn't make it on time for 2.7 feature freeze. So I'm opening the new PR for 2.8.

Following is my first introduction:

> In the small job which needs not to build a form (e.g. just process a value gotten from Request), I want to reuse the trim logic.

@dosten
@Tobion
@xabbuh
@stof
@webmozart
Thanks for all your feedback. I have fixed all issues mentioned on #14146. Please re-consider it.

Commits
-------

f42c777 [Form] moved data trimming logic of TrimListener into StringUtil
@issei-m issei-m deleted the form-string-util branch May 16, 2016 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@issei-m @dosten @xabbuh @webmozart @stof @Tobion