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] Remove deprecated case insensitive service ids#22811

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

Closed
ro0NL wants to merge2 commits intosymfony:masterfromro0NL:di/service-ids-4.0
Closed

[DI] Remove deprecated case insensitive service ids#22811

ro0NL wants to merge2 commits intosymfony:masterfromro0NL:di/service-ids-4.0

Conversation

ro0NL
Copy link
Contributor

@ro0NLro0NL commentedMay 20, 2017•
edited
Loading

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

See#21223

Koc and sstok reacted with heart emoji
4.0.0
-----

* [BC BREAK] removed support for case insensitive service identifiers

Choose a reason for hiding this comment

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

BC BREAK prefix not needed

@nicolas-grekas
Copy link
Member

I think we need a proper behavior + test when two services are defined (ie pre-compile, using ContainerBuilder) and they have an id that differs only by the case: either it works properly (does it already?) or fail early (might be more DX friendly?)

ro0NL, sstok, and apfelbox reacted with thumbs up emojisstok reacted with hooray emoji

@ro0NL
Copy link
ContributorAuthor

ro0NL commentedMay 21, 2017•
edited
Loading

method map seems prepared :)

         $this->methodMap = array(-            'bar' => 'getBarService',+            'BAR' => 'getBARService',+            'BAR2' => 'getBAR2Service',+            'bar' => 'getBar3Service',+            'bar2' => 'getBar22Service',

will add one more test for PhpDumper :) but i think it works properly 👍

@@ -30,6 +30,9 @@ digraph sc {
node_lazy_context [label="lazy_context\nLazyContext\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_lazy_context_ignore_invalid_ref [label="lazy_context_ignore_invalid_ref\nLazyContext\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_closure_proxy [label="closure_proxy\nBarClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_bar [label="BAR\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_bar2 [label="bar2\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_bar2 [label="BAR2\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
Copy link
ContributorAuthor

@ro0NLro0NLMay 21, 2017•
edited
Loading

Choose a reason for hiding this comment

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

not sure this is actually needs to be lowercased, perhaps qualifies a bugfix (2.7).

edit: or master / this PR; as it's an issue as of now. Let me know.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this needs to be fixed as part of this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Graphviz nodes seem to be case-sensitive. so removing the lowercase should fix this.

ro0NL reacted with thumbs up emoji
@ro0NL
Copy link
ContributorAuthor

@nicolas-grekas geen! new tests are proper imo. and allows for future edge cases to be added. I think we should at least try out this behavior on master =/

Changes toContainer.php best reviewed athttps://github.com/symfony/symfony/pull/22811/files?w=1#diff-1df5b32be44d0f592ec285c6bbf386bb

fabbot.io failure seems unrelated :)

Ready for review.

@nicolas-grekas
Copy link
Member

rebase needed

@@ -1140,6 +1140,21 @@ public function testRegisterForAutoconfiguration()
// when called multiple times, the same instance is returned
$this->assertSame($childDefA, $container->registerForAutoconfiguration('AInterface'));
}

public function testCaseInsensitivity()
Copy link
Contributor

Choose a reason for hiding this comment

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

should be testCaseSensitivity

ro0NL reacted with laugh emoji
* @expectedDeprecation Service identifiers will be made case sensitive in Symfony 4.0. Using "Foo" instead of "foo" is deprecated since version 3.3.
*/
public function testGetInsensitivity()
public function testCaseInsensitivity()
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@@ -49,7 +49,7 @@ protected function resetService($name)
}
$manager->setProxyInitializer(\Closure::bind(
function (&$wrappedInstance, LazyLoadingInterface $manager) use ($name) {
if (isset($this->aliases[$name = strtolower($name)])) {
if (isset($this->aliases[$name])) {
$name = $this->aliases[$name];
}
$method = !isset($this->methodMap[$name]) ? 'get'.strtr($name, $this->underscoreMap).'Service' : $this->methodMap[$name];
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@stof not sure here.. it seems forgotten? This is the only use-case ofContainer::$underscoreMap left.

Copy link
Member

Choose a reason for hiding this comment

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

methodMap will always be filled in 4.0 (we removed support for dumping a container with a custom dumper not dumping the method map)

@ro0NL
Copy link
ContributorAuthor

ro0NL commentedMay 26, 2017•
edited
Loading

should i remove the (deprecated)FactoryReturnTypePass while at it? removing the strtolower calls breaks test :)

cc@GuilhemN

@GuilhemN
Copy link
Contributor

GuilhemN commentedMay 26, 2017•
edited
Loading

method map seems prepared :)

#18167 indeed added support for sensitivity while fixing the PhpDumper for non alphanumeric ids.

should i remove the (deprecated) FactoryReturnTypePass while at it?

I think it would be easier to review if it was done in another PR. Nothing would prevent you from merging this PR here though.

@ro0NL
Copy link
ContributorAuthor

Reverted compiler passes, they need to have a look though :) as well as theContainer::$underscoreMap thingy.

Other then that, i think this is ready :)

@@ -49,7 +49,7 @@ protected function resetService($name)
}
$manager->setProxyInitializer(\Closure::bind(
function (&$wrappedInstance, LazyLoadingInterface $manager) use ($name) {
if (isset($this->aliases[$name = strtolower($name)])) {
if (isset($this->aliases[$name])) {
$name = $this->aliases[$name];
}
$method = !isset($this->methodMap[$name]) ? 'get'.strtr($name, $this->underscoreMap).'Service' : $this->methodMap[$name];
Copy link
Member

Choose a reason for hiding this comment

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

methodMap will always be filled in 4.0 (we removed support for dumping a container with a custom dumper not dumping the method map)

@@ -49,7 +49,7 @@ protected function resetService($name)
}
$manager->setProxyInitializer(\Closure::bind(
function (&$wrappedInstance, LazyLoadingInterface $manager) use ($name) {
if (isset($this->aliases[$name = strtolower($name)])) {
Copy link
Member

Choose a reason for hiding this comment

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

this requires a bugfix in 3.3 first instead: we need to use thenormalizedIds property here instead of lowercasing (internal storage does not use lowercase anymore)

@fabpot
Copy link
Member

@ro0NL What's the status of this PR?

@ro0NL
Copy link
ContributorAuthor

@fabpot i should rebase at least :) let me have a look tomorrow evening / saturday.

Status: needs work

@ro0NL
Copy link
ContributorAuthor

@fabpot looks good to me 👍

?w=1

Status: needs review

@@ -28,7 +28,7 @@
*
* Parameter and service keys are case insensitive.
*
* A service id can containlowercasedletters, digits, underscores, and dots.
* A service id can contain letters, digits, underscores, and dots.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem correct anymore. Service ids are not actually validated are they? They can also contain namespace backslashes for example.

Copy link
Contributor

@GuilhemNGuilhemNJul 7, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Imo this comment should be removed, a service id can indeed contain any character.

jvasseur reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

My guess is this was written from a file loader pov, As in fact any character is allowed as of 2.7.

@@ -28,7 +28,7 @@
*
* Parameter and service keys are case insensitive.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not up-to-date as well

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Fixed.

@@ -28,7 +28,7 @@
*
* Parameter and service keys are case insensitive.
*
* A service id can containlowercasedletters, digits, underscores, and dots.
* A service id can contain letters, digits, underscores, and dots.
* Underscores are used to separate words, and dots to group services
Copy link
Contributor

Choose a reason for hiding this comment

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

this should also mention that this is only relevant when you have several services of the same class. otherwise you can use service id == class name.

Copy link
ContributorAuthor

@ro0NLro0NLJul 8, 2017•
edited
Loading

Choose a reason for hiding this comment

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

I removed the whole block =/ This is just some service id convention, not really related to the container at all; as it allows any character as mentioned.

The part about creating a method is outdated, as it only checks the method map now.

@nicolas-grekas
Copy link
Member

Thank you@ro0NL.

@ro0NLro0NL deleted the di/service-ids-4.0 branchJuly 12, 2017 11:22
fabpot added a commit that referenced this pull requestJul 17, 2017
This PR was merged into the 2.7 branch.Discussion----------[DI] Remove irrelevant comment from container| Q             | A| ------------- | ---| Branch?       | 2.7| Bug fix?      | yes-ish| 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-->Spotted in#22811Commits-------595a225 [DI] Remove irrelevant comment from container
@fabpotfabpot mentioned this pull requestOct 19, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@GuilhemNGuilhemNGuilhemN left review comments

@stofstofstof requested changes

@TobionTobionTobion approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

7 participants
@ro0NL@nicolas-grekas@GuilhemN@fabpot@stof@Tobion@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp