10000 [RFC] Rename some HeaderUtils methods · Issue #27026 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC] Rename some HeaderUtils methods #27026

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
javiereguiluz opened this issue Apr 24, 2018 · 8 comments
Closed

[RFC] Rename some HeaderUtils methods #27026

javiereguiluz opened this issue Apr 24, 2018 · 8 comments
Labels
HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@javiereguiluz
Copy link
Member
Q A
Bug report? no
Feature request? no
BC Break report? no
RFC? yes
Symfony version 4.1

I'd like to propose to rename some methods of HeaderUtils before Symfony 4.1 is released.

split()

I don't like that name because PHP removed the split() function in PHP 7.0. Could we rename it to explode() to be inline with modern PHP naming?

combineParts()

I wonder if combine() would be enough. Maybe not. It's just that this method name is not "symmetrical" to split() (which should be called splitParts() then).

joinAssoc()

This is the method name I like the least. I've looked up in the "Header Fields" section of HTTP standard (https://tools.ietf.org/html/rfc7230#section-3.2) but there's no terminology about headers that we could use here.

I've looked up in other frameworks and I haven't found much. Spring for example calls this
toCommaDelimitedString()

The problem I see with joinAssoc() name is that it explains the low level details of how it works (joining associative arrays into a single string). I'd prefer a method that describes what it does: stringify(), toString(), asString(), etc.
@javiereguiluz javiereguiluz added HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Apr 24, 2018
@Simperfit
Copy link
Contributor

  1. I agree with you, it should be explode to fit with modern php.

  2. if we name the first explode, can't we rename this one implode?

  3. the problem with stringify is we don't know that it stringifies multiple associative arrays, why not something like JoinAssocToString ?

@linaori
Copy link
Contributor
linaori commented Apr 24, 2018

I'm personally not a fan of the name explode, as it reminds me of violence, so I'm 👎 on that one. I think split is a more accurate description, despite not being in line with the php function name. splitParts sounds really decent in conjunction with combineParts.

@javiereguiluz javiereguiluz mentioned this issue Apr 24, 2018
Closed
@nicolas-grekas
Copy link
Member

ping @c960657 if you'd like to apply some ideas in #27019

@QuentinCurtet
Copy link
Contributor
  1. For the explode name, it could cause some misunderstanding because the name is already use by PHP and the parameters are not in the same way that the PHP method. I join @iltar on the splitParts.
  2. I wouldn't change this because of the renaming of split to splitParts
  3. The method only takes nested arrays (not integer or other), so the toString method is too generic, why not nestedArraysToString ?

@c960657
Copy link
Contributor
c960657 commented Apr 25, 2018

It's been a while since I wrote the patch, so I no longer remember exactly what went through my head back then :-) I did give it some consideration, but I wasn't too happy with the result.

The thing is that the functions are not inverse, like PHP's implode/explode are.

split takes a string and turns it into a nested array structure. Each array (on all levels) are referred to as “parts” (in lack of a better name).

If, and only if, there are at least two separator characters and the last is =, the parts on the second-deepest level in the array structure may be converted to an associative array using combineParts, and that may in turn be converted back to a string using joinAssoc.

It seems that split and combineParts are often used in pairs, so it is probably a good idea to keep those symmetric. I am happy with all suggestions mentioned here. I may prefer just split/combine, because “part” is not a well-known term or concept elsewhere in Symfony or in the outside world.

I'm not so fond of the name splitParts. It doesn't split parts, but splits a string into parts (at least if “part” refers to the array structure accepted by combineParts).

An alternative name for joinAssoc might be toHeader or toString. I don't think it matters that the name is generic – there are (currently) no other functions in the class that generate strings.

Anyway, I'll be happy to add whatever decision we come up with to #27019 (or a separate PR).

@nicolas-grekas
Copy link
Member

This needs to be taken care of asap, before 4.1 is tagged. Any decision?

@javiereguiluz
Copy link
Member Author

@nicolas-grekas do you have some ideas for this possible renaming? After reading @c960657's comment it looks like split(), combine() and toString() could be good candidates.

@c960657 c960657 mentioned this issue Apr 29, 2018
Merged
@c960657
Copy link
Contributor
c960657 commented Apr 29, 2018 •
edited
Loading 8F51

I have created PR #27087 that changes the names to split(), combine() and toString().

I do not mean to shortcut the discussion. I have just prepared the PR to speed up things, if there is consensus about the suggested renaming.

@fabpot fabpot closed this as completed Apr 30, 2018
fabpot added a commit that referenced this issue Apr 30, 2018
@fabpot
minor #27087 [HttpFoundation] Rename HeaderUtils methods (c960657)
295eaed 
This PR was merged into the 4.1-dev branch.

Discussion
----------

[HttpFoundation] Rename HeaderUtils methods

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

Rename new HeaderUtils methods as discussed in #27026.

Commits
-------

484d1fb [HttpFoundation] Rename HeaderUtils methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@fabpot @javiereguiluz @c960657 @nicolas-grekas @linaori @Simperfit @QuentinCurtet
0