Only allow lowercase service and parameter names [#2498293]

Soft-Dependency for #2497243: Replace Symfony container with a Drupal one, stored in cache:

Problem/Motivation

Core uses once 'persistIds' and once 'form_test.form.serviceForm' as the only uppercase services (and here relies on a side-effect), but Symfony internally lower cases everything, so anything dumped will be 'persistids' and 'form_test.form.serviceform'.

(Symfony does that because it camelCase's at run time, which is bad for performance.)

Proposed resolution

Deprecate using camelCase, use "_" everywhere with lowercase in container service IDs or parameter names.

Throw an Exception in ContainerBuilder to support developers in fixing their applications.

Remaining tasks

- Patch
- Fix it

User interface changes

- None

API changes

- camelCase service IDs or parameters are no longer supported.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category	Task because this is a performance enhancement.
Issue priority	Major because it is a non-Critical performance issue whose time savings have not been demonstrated.
Prioritized changes	The main goal of this issue is performance.
Disruption	It will be a minor disruption for contrib modules who may use uppercased names. The exception this patch throws is clear and obvious. If we do not commit this now, we will not have another chance during the 8.x cycle.

Comment	File	Size	Author
#17	only_allow_lowercase-2498293-17.patch	5.28 KB	cilefen
#17
#17	interdiff.txt	1.66 KB	cilefen
#17	only_allow_lowercase-2498293-17-tests.patch	1.4 KB	cilefen
#17
#13	only_allow_lowercase-2498293-13.patch	5.32 KB	cilefen
#13
#13	interdiff.txt	625 bytes	cilefen
#13	only_allow_lowercase-2498293-13-tests.patch	1.43 KB	cilefen
#13
#6	only_allow_lowercase-2498293-6.patch	5.04 KB	cilefen
#6
#6	only_allow_lowercase-2498293-6-tests.patch	1.16 KB	cilefen
#6
#6	interdiff.txt	1.42 KB	cilefen
#3	only_allow_lowercase-2498293-2.patch	3.83 KB	cilefen
#3
#3	only_allow_lowercase-2498293-2-test.patch	886 bytes	cilefen
#3
#3	interdiff.txt	720 bytes	cilefen
#1	only_allow_lowercase-2498293-1.patch	2.97 KB	Fabianx
#1
#1	only_allow_lowercase-2498293-1--test-only.patch	1.48 KB	Fabianx
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented 1 June 2015 at 13:37

File	Size
only_allow_lowercase-2498293-1--test-only.patch	1.48 KB

only_allow_lowercase-2498293-1.patch	2.97 KB

And a simple patch ...

Comment #2

1 June 2015 at 13:41

The last submitted patch, 1: only_allow_lowercase-2498293-1--test-only.patch, failed testing.

Comment #3

cilefen CreditAttribution: cilefen commented 1 June 2015 at 13:46

File	Size
interdiff.txt	720 bytes
only_allow_lowercase-2498293-2-test.patch	886 bytes

only_allow_lowercase-2498293-2.patch	3.83 KB

I think this is where we need a test.

Comment #4

1 June 2015 at 14:08

The last submitted patch, 1: only_allow_lowercase-2498293-1.patch, failed testing.

Comment #5

1 June 2015 at 14:24

Status:

Needs review

» Needs work

The last submitted patch, 3: only_allow_lowercase-2498293-2.patch, failed testing.

Comment #6

cilefen CreditAttribution: cilefen commented 1 June 2015 at 14:32

Status:

Needs work

» Needs review

File	Size
interdiff.txt	1.42 KB
only_allow_lowercase-2498293-6-tests.patch	1.16 KB

only_allow_lowercase-2498293-6.patch	5.04 KB

There was another: DefaultConfigTest.schema_storage

Comment #7

dawehner

German

CreditAttribution: dawehner as a volunteer and for Drupal Association commented 1 June 2015 at 14:42

That is odd, isn't the point that you can in HEAD access the services by upper and lowercase name.
So why not just tell people: You can't access services, with a lowercase name, in case you have supported uppercase at some point.

Having said that, I think its still a fair limitation to have just lowercase names.

Comment #8

1 June 2015 at 14:53

The last submitted patch, 3: only_allow_lowercase-2498293-2-test.patch, failed testing.

Comment #9

1 June 2015 at 15:01

The last submitted patch, 6: only_allow_lowercase-2498293-6-tests.patch, failed testing.

Comment #10

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented 1 June 2015 at 16:47

We might need some more test coverage for the other methods?

Comment #11

lauriii

he/him

Finnish

Finland

CreditAttribution: lauriii at Druid commented 1 June 2015 at 16:47

Issue tags:

+Needs change record, +Needs beta evaluation

Comment #12

cilefen CreditAttribution: cilefen commented 1 June 2015 at 16:49

@Fabianx Oh, ::register()...

Comment #13

cilefen CreditAttribution: cilefen commented 1 June 2015 at 16:55

File	Size
only_allow_lowercase-2498293-13-tests.patch	1.43 KB

interdiff.txt	625 bytes
only_allow_lowercase-2498293-13.patch	5.32 KB

Comment #14

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented 1 June 2015 at 17:20

And ::set

Comment #15

cilefen CreditAttribution: cilefen commented 1 June 2015 at 17:23

@Fabianx ::set() is there.

Comment #16

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented 1 June 2015 at 17:55

+++ b/core/tests/Drupal/Tests/Core/DependencyInjection/ContainerBuilderTest.php
@@ -30,12 +30,37 @@ public function testGet() {
    * @covers ::set
+   * @expectedException \InvalidArgumentException
+   * @expectedExceptionMessage Service ID names must be lowercase: Bar
    */
...
+    $class = new BarClass();
+    $container->set('Bar', $class);
+    $this->assertNotEquals('bar', $class->_serviceId);
+  }

Should be in its own function to isolate the test case ...

Comment #17

cilefen CreditAttribution: cilefen commented 1 June 2015 at 19:52

File	Size
only_allow_lowercase-2498293-17-tests.patch	1.4 KB

interdiff.txt	1.66 KB
only_allow_lowercase-2498293-17.patch	5.28 KB

Comment #18

2 June 2015 at 00:45

The last submitted patch, 13: only_allow_lowercase-2498293-13-tests.patch, failed testing.

Comment #19

2 June 2015 at 03:08

The last submitted patch, 17: only_allow_lowercase-2498293-17-tests.patch, failed testing.

Comment #20

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented 2 June 2015 at 07:47

Assigned:

Fabianx

» Unassigned

Great work, still need a beta eval and change record. I don't have time right now, but will hopefully come back to this later again sometime this week.

Edit:

Would be RTBC - if not missing that.

Comment #21

cilefen CreditAttribution: cilefen commented 2 June 2015 at 12:35

Draft CR.

Comment #22

cilefen CreditAttribution: cilefen commented 2 June 2015 at 12:41

Issue summary:	View changes
Issue tags:	-Needs change record, -Needs beta evaluation

Comment #23

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented 2 June 2015 at 19:38

Assigned:	Unassigned	» catch
Status:	Needs review	» Reviewed & tested by the community

Lets get core committer feedback.

Comment #24

4 June 2015 at 14:11

catch committed c219ec8 on 8.0.x

Issue #2498293 by cilefen, Fabianx: Only allow lowercase service and...

Comment #25

catch

he/him

English

CreditAttribution: catch commented 4 June 2015 at 14:13

Status:

Reviewed & tested by the community

» Fixed

Yes this constraint makes sense and while I don't think we can remove the complexity added upstream to support case insensitivity here at least we can ensure we're not relying on that behaviour ourselves - lets us optimize without breaking backwards compatibility later.

Committed/pushed to 8.0.x, thanks!