Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DI] Case sensitive parameter names#23874

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

Merged
fabpot merged 1 commit intosymfony:3.4fromro0NL:di/params
Aug 22, 2017
Merged

[DI] Case sensitive parameter names#23874

fabpot merged 1 commit intosymfony:3.4fromro0NL:di/params
Aug 22, 2017

Conversation

@ro0NL
Copy link
Contributor

@ro0NLro0NL commentedAug 12, 2017•
edited
Loading

QA
Branch?3.4
Bug fix?no
New feature?no
BC breaks?no
Deprecations?yes
Tests pass?yes
Fixed tickets#23809
LicenseMIT
Doc PRsymfony/symfony-docs#...

@GuilhemN took your patch.. but i use the same deprecation messages as for case sensitive service id's, i found it more clear. Also comparing to $origName to keep the diff smaller

@GuilhemN
Copy link
Contributor

Looks good so far 👍 thanks for taking care of this.

*/
publicstaticfunctionnormalizeName($name)
{
if (0 ===strpos($name,'env(') &&')' ===substr($name, -1) &&'env()' !==$name) {
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@nicolas-grekas can you validate this approach? We dont want a deprecation for env(NAME) i guess =/


$normalizedName =strtolower($name);
if ($normalizedName !== (string)$name) {
@trigger_error(sprintf('Parameter names will be made case sensitive in Symfony 4.0. Using "%s" instead of "%s" is deprecated since version 3.4.',$name,$normalizedName),E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

So this basically means that I get a deprecation message whenever I use uppercase characters in parameter names? This is kind of misleading because uppercase letters won't be forbidden in Symfony 4, will they?

I mean, if I set a parameter likemyCamelCaseParameter and always usemyCamelCaseParameter to fetch it from the bag, I don't see a problem with that, although it might not be best practice to format parameter names that way.

The problematic workflow would be to setmyCamelCaseParameter and requestmycamelcaseparameter afterwards. Because, as soon as parameter names are treated case-sensitively, this piece of code is going to break. And imho only in this case, I should get a deprecation warning.

jvasseur reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

That's what was done for services name but the complexity to do it is not worth it for parameters imo. If you don't want to have these deprecations, you'll still be able to quickly upgrade to 4.0.

Copy link
Member

@chalasrchalasrAug 12, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Saying "don't use uppercase letters" in order to allow them later looks weird to me as well. I would expectset() to not alter$name andget() to trigger if the lowercased name of an existing parameter equals$name (better put effort in complex BC layers than give complex upgrade paths to our users)

derrabus and jvasseur reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

Deprecation warnings in Symfony are DX feature, used to point out code that is going to break on update. If we produce false positives, we make it harder for users to detect this kind of issues. It's hard to see, let's say, 2 real issues if they're hidden among 198 false positives.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chalasr@derrabus that's fair points. I'll give it a try.

chalasr and derrabus reacted with thumbs up emoji
Copy link
Contributor

@sstoksstokAug 12, 2017•
edited
Loading

Choose a reason for hiding this comment

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

What if you keep a list of all parameters (that are not in lowercase), then when fetching a parameter (in normalized format) check of the casing differs with the original? if there is no difference you don't have to throw an deprecation warning.

In practice keeping a list of normalized names (like we do for private id's atm).

Copy link
Contributor

Choose a reason for hiding this comment

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

@ro0NL you can do it of course, that's your PR :)

I think we should keep$this->parameters as is and store original keys in a new property such as$this->rawKeys, the difficulty is to keep this property after compilation.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah.. im already working on the same approach as done for service id's 👍

sstok reacted with hooray emoji
Copy link
ContributorAuthor

@ro0NLro0NLAug 12, 2017•
edited
Loading

Choose a reason for hiding this comment

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

What do we expect fromall().. should the keys stay lowercase?

edi: this is a bit more work :) i found out.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ro0NL having keys in their original case would make more sense with this change imo. That's a small bc break but I think it's acceptable.

@ro0NL
Copy link
ContributorAuthor

I think this will do 👍

return $normalizedName;
EOF;
}else {

Choose a reason for hiding this comment

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

in this case, no deprecation is triggered, isnt it?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed!

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Almost good. There is one case that needs more care:
resolving env placeholders should be case insensitive, because that's purely internal and compile time, and that'll allow them to work through DI ext transformations (some do "strtolower" which would be fine already, but others might do "strtoupper", which should continue to work)

thrownewInvalidArgumentException(sprintf('Parameter name cannot use env parameters: %s.',$resolvedKey));
}
if ($key !==$lcKey =strtolower($key)) {
$normalizedParams[] =str_repeat('',8).sprintf('%s => %s,',$this->export($lcKey),$this->export($key));

Choose a reason for hiding this comment

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

not sure using str_repeat is better than putting spaces in the sprintf (same below)

EOF;

$code .=' private $normalizedParameterNames ='.($normalizedParams ?sprintf("array(\n%s\n%s);",implode("\n",$normalizedParams),str_repeat('',4)) :'array();')."\n";

Choose a reason for hiding this comment

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

str_repeat? (etc if more :) )

private function normalizeParameterName($name)
{
if (!is_string($name)) {

Choose a reason for hiding this comment

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

the cast should be unconditional, the engine already takes care of this "if"

privatefunctionnormalizeParameterName($name)
{
if (!is_string($name)) {
$name = (string)$name;

Choose a reason for hiding this comment

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

should be unconditional

/**
* @internal to be removed in 4.0
*/
publicfunctionnormalizeName($name)

Choose a reason for hiding this comment

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

why public?

@ro0NL
Copy link
ContributorAuthor

ro0NL commentedAug 14, 2017•
edited
Loading

@nicolas-grekas that means we keepnormalizeParameterName in 4.0 right? Always checking forenv() syntax...

The request contradicts with params being case sensitive though.. do we really care about "DI transformations"? Also in bash vars are case sensitive... given that you get exactly the right upgrade path currently.

@nicolas-grekas
Copy link
Member

OK, reread again, works already (I was talking about env "placeholders", not env "references", if that distinction makes sense - see str_ireplace in ContainerBuilder::resolveEnvPlaceholders(), which is fine)

ro0NL reacted with thumbs up emoji

private function normalizeParameterName($name)
{
if (isset($this->normalizedParameterNames[$normalizedName = strtolower($name)]) || isset($this->parameters[$normalizedName]) || array_key_exists($normalizedName, $this->parameters)) {
$normalizedName = isset($this->normalizedParameterNames[$normalizedName]) ? $this->normalizedParameterNames[$normalizedName] : $normalizedName;
Copy link
Contributor

Choose a reason for hiding this comment

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

that is not the normalized (lowercased) parameter name, but the original name. using the same variable for both things is confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

Or it might be easier to rename the variable$normalizedName = strtolower($name) to$lowercasedName to remove the duplicate usage

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

not sure we care.. it's optimized code. I believe this will be a hot code path.

@nicolas-grekas
Copy link
Member

Status: reviewed

@fabpot
Copy link
Member

Thank you@ro0NL.

@fabpotfabpot merged commit8a1d168 intosymfony:3.4Aug 22, 2017
fabpot added a commit that referenced this pull requestAug 22, 2017
This PR was merged into the 3.4 branch.Discussion----------[DI] Case sensitive parameter names| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets |#23809| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->@GuilhemN took your patch.. but i use the same deprecation messages as for case sensitive service id's, i found it more clear. Also comparing to $origName to keep the diff smallerCommits-------8a1d168 [DI] Case sensitive parameter names
@TomasVotruba
Copy link
Contributor

Great job@ro0NL and@GuilhemN , thank you

nicolas-grekas added a commit that referenced this pull requestAug 31, 2017
This PR was merged into the 3.4 branch.Discussion----------[DI] Add upgrade note about case insenstive params| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->Forgotten in#23874 :)PS: im working on a PR for 4.0 as wellCommits-------3890996 [DI] Add upgrade note about case insenstive params
This was referencedOct 18, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@derrabusderrabusderrabus left review comments

@chalasrchalasrchalasr left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+3 more reviewers

@TobionTobionTobion left review comments

@sstoksstoksstok left review comments

@GuilhemNGuilhemNGuilhemN left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

10 participants

@ro0NL@GuilhemN@nicolas-grekas@fabpot@TomasVotruba@Tobion@sstok@derrabus@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp