Skip to content

[DependencyInjection] Removed useless strtolower call #14211

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 4, 2015
Merged

[DependencyInjection] Removed useless strtolower call #14211

merged 1 commit into from
Apr 4, 2015

Conversation

dosten
Copy link
Contributor

@dosten dosten commented Apr 4, 2015

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

The setDefinition method already lowercase the id, so it's not necessary to lowercase it twice.

@wouterj
Copy link
Member

wouterj commented Apr 4, 2015

👍

@javiereguiluz
Copy link
Member

Just a suggestion: without giving more context information it's hard to understand why this strtolower call is useless.

@dosten
Copy link
Contributor Author

dosten commented Apr 4, 2015

@javiereguiluz fixed!

@aitboudad
Copy link
Contributor

👍

@javiereguiluz
Copy link
Member

@dosten thanks! Your proposal makes a lot of sense now :)

@fabpot
Copy link
Member

fabpot commented Apr 4, 2015

Thank you @dosten.

@fabpot fabpot merged commit f62b050 into symfony:2.3 Apr 4, 2015
fabpot added a commit that referenced this pull request Apr 4, 2015
…osten)

This PR was merged into the 2.3 branch.

Discussion
----------

[DependencyInjection] Removed useless strtolower call

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

The `setDefinition` method already lowercase the id, so it's not necessary to lowercase it twice.

Commits
-------

f62b050 Removed useless strtolower call
@dosten dosten deleted the useless-strtolower branch April 4, 2015 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants