Skip to content
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

[DI] Deprecate case insentivity of service identifiers #21223

Merged
merged 1 commit into from Jan 11, 2017

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Copy link
Member

commented Jan 10, 2017

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? minor (see UPGRADE note)
Deprecations? yes
Tests pass? yes
Fixed tickets #21193
License MIT
Doc PR -

As discussed in linked RFC.

@@ -51,12 +51,12 @@ public function process(ContainerBuilder $container)
private function updateDefinition(ContainerBuilder $container, $id, Definition $definition, array $resolveClassPassChanges, array $previous = array())
{
// circular reference
if (isset($previous[$id])) {
if (isset($previous[$lcId = strtolower($id)])) {

This comment has been minimized.

Copy link
@stof

stof Jan 10, 2017

Member

I would move $lcId = strtolower($id) on its own statement. It would be more readable

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 10, 2017

Author Member

fixed, other comments addressed also

@@ -407,7 +398,7 @@ public function removeDefinition($id)
*/
public function has($id)
{
$id = strtolower($id);
$id = $this->normalizeId($id);
return isset($this->definitions[$id]) || isset($this->aliasDefinitions[$id]) || parent::has($id);

This comment has been minimized.

Copy link
@stof

stof Jan 10, 2017

Member

shoudln't we pass the original value to the parent call ?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 10, 2017

Author Member

this would only trigger a second deprecation, nothing else

@@ -1246,8 +1221,8 @@ private function callMethod($service, $call)
*/
private function shareService(Definition $definition, $service, $id)
{
if ($definition->isShared()) {
$this->services[$lowerId = strtolower($id)] = $service;
if (null !== $id && $definition->isShared()) {

This comment has been minimized.

Copy link
@stof

stof Jan 10, 2017

Member

when can it be null ?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 10, 2017

Author Member

because of eg $this->createService($value, null) in the code

@@ -29,7 +29,7 @@ class Reference
*/
public function __construct($id, $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE)
{
$this->id = strtolower($id);
$this->id = $id;

This comment has been minimized.

Copy link
@stof

stof Jan 10, 2017

Member

we still need to cast to string here (with a deprecation), otherwise __toString will be broken

This comment has been minimized.

Copy link
@stof

stof Jan 10, 2017

Member

same for Alias btw

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-case-sensitive branch 5 times, most recently from 99f285f to 9e1dbff Jan 10, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 10, 2017

PR ready for second review

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-case-sensitive branch from 9e1dbff to 99083df Jan 10, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

You need to also update UPGRADE-4.0.md

@@ -22,7 +22,12 @@ class Alias
*/
public function __construct($id, $public = true)
{
$this->id = strtolower($id);
if (!is_string($id)) {

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Jan 10, 2017

Contributor

Do we really need this? The docblock already specifies that it must be string.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 10, 2017

Author Member

That's what was said also about container->get, then I had to explicitly add string casts in our own tests. So, yes: a BC layer is a friendly default policy.

This comment has been minimized.

Copy link
@GuilhemN

GuilhemN Jan 10, 2017

Contributor

Ok, fine for me :)

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jan 10, 2017

Contributor

@nicolas-grekas just finished mine :).. probably related. Not sure i follow.. but ill dig in :)

if (!is_string($id)) {
$type = is_object($id) ? get_class($id) : gettype($id);
$id = (string) $id;
@trigger_error(sprintf('Non-string service identifiers are deprecated since Symfony 3.3 and won\'t be supported in 4.0 for service "%s" ("%s" given.) Cast it to string beforehand.', $id, $type), E_USER_DEPRECATED);

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jan 10, 2017

Contributor

At this point we dont really know what the case of the non-string-service-id is right? Meaning this deprecation can be a false positive?

Ie. $normalizedId = strtolower((string) $id) looks sufficient? Triggering deprecation as usual from below..

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 10, 2017

Author Member

You should read the deprecation message twice, same for your other comments :)

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jan 10, 2017

Contributor

Holy crap.. i got it :) Sorry.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-case-sensitive branch from 99083df to d08f110 Jan 11, 2017

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2017

UPGRADE-4.0.md updated

@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit d08f110 into symfony:master Jan 11, 2017

2 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request Jan 11, 2017

feature #21223 [DI] Deprecate case insentivity of service identifiers…
… (nicolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Deprecate case insentivity of service identifiers

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | minor (see UPGRADE note)
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #21193
| License       | MIT
| Doc PR        | -

As discussed in linked RFC.

Commits
-------

d08f110 [DI] Deprecate case insentivity of service identifiers

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-case-sensitive branch Jan 13, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

nicolas-grekas added a commit that referenced this pull request Jul 11, 2017

feature #22811 [DI] Remove deprecated case insensitive service ids (r…
…o0NL)

This PR was squashed before being merged into the 4.0-dev branch (closes #22811).

Discussion
----------

[DI] Remove deprecated case insensitive service ids

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| 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-->

See #21223

Commits
-------

63e26fc [DI] Remove deprecated case insensitive service ids
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.