[DI] Randomize private services #19683

ro0NL · 2016-08-20T11:16:10Z

Q	A
Branch?	"master"
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	not yet
Fixed tickets	#19680
License	MIT
Doc PR	reference to the documentation PR, if any

Alternative approach compared to #19682

Thanks to @wouterj for explaining this approach.

Theory of operation;

compiler pass
- build a map of private id => random id
- remap private definitions to its new id
- update references and aliases
- inject id map into container builder (for BC)
container
- id's found in the (injected) map are forwarded to the new id + deprecation notice
todo
- add tests
- fix tests due randomization
- dump the container with orignal id's in generated docs, etc. for convenience.

The deprecation now only happens after compilation(!). This can be seen a design choice, i guess.

ro0NL · 2016-08-20T11:21:49Z

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -839,6 +839,7 @@ public function __construct()

        $code .= "\n        \$this->services = array();\n";
        $code .= $this->addMethodMap();
+        $code .= $this->addPrivateServices();


Needed to trigger deprecations in get for a compiled container when dumped.

wouterj · 2016-08-20T11:39:50Z

This is basically a big copy/paste from the original get() function, right? I don't like that too much (it gets out of sync very quick and requires more work when some code is changed in get()/resolveService()).

Also, please note that in Symfony 4, private services will be implemented by using a randomly generated service ID. So implementing a complete new API for private services isn't needed.

ro0NL · 2016-08-20T11:45:38Z

This is about separating (concerns), not copy-paste. Basically this makes get final, have a look at the diff.

About 4.x changes.. that is eventually still future talk. I compare against the current codebase.

Even then, if you have a "random id" you need to be able to get it internally, without deprecations. That would only cleanup get.. unless you make a real good guess ;-) which should still, eh trigger.

ro0NL · 2016-08-20T12:35:21Z

Im not even sure we need random id's, if it's bulletproof already.

sstok · 2016-08-20T12:37:07Z

Im not even sure we need random id's, if it's bulletproof already.

Using random id's makes it possible to use same logic, it be like using a normal id except you don't know the id.

Is it possible to benchmark these changes with Blackfire.io? I'm worried performance could be degraded.

wouterj · 2016-08-20T12:44:11Z

Even then, if you have a "random id" you need to be able to get it internally, without deprecations. That would only cleanup get.. unless you make a real good guess ;-) which should still, eh trigger.

A compiler would generate random IDs for private services and fix the service definitions that depend on them. No need to change anything in the Container.

ro0NL · 2016-08-20T12:48:03Z

@wouterj Fair enough 👍

I just dont understand how that reflects dumping..;

generate random id
update references
a private service needs to be injected by some id
- are we gonna bypass the container to get the object?

@sstok i think it's boosted (we bypass get many times now).. but im not sure. Not familiar yet with blackfire.io.. could you collab?

wouterj · 2016-08-20T13:01:20Z

I just dont understand how that reflects dumping..;

Compiler finds private service
Generates random ID
Updates all references to the old ID with the generated one
Container is dumped, using the random ID for the service, just like normal.

As said by @sstok, the only difference between a private and a public service will be that you don't know the ID of the private one (as it's automatically generated).

sstok · 2016-08-20T13:03:24Z

You could start by randomizing them already, and use those. But keep an alias array with private services, pointing to the actual id 👍

ro0NL · 2016-08-20T13:06:59Z

Based on the current design;

php dumper generates calls to (private) services using get(). How will that not trigger/throw,
unless we alter get to allow for privates? Again; are we gonna bypass the container getter?
having a list of private id's (randomized or not) is useful
randomizing id's is a next step

wouterj · 2016-08-20T13:08:35Z

php dumper generates calls to (private) services using get(). How will that not trigger/throw? Again are gonna bypass the container methods?

If requested service is the randomized ID, it's an internal call and thus should not trigger deprecations. If it uses the original service ID, it's a call from outside world and the deprecation should be triggered (and randomized service returned).

E.g:

public function get($id, ...)
{
    if (isset($this->privates[$id])) {
        @trigger_error(..., E_USER_DEPRECATED);

        $id = $this->privates[$id];
    }

    // ... behave like normal, $id is either a public service or the randomized ID
}

ro0NL · 2016-08-20T13:15:10Z

Ok, that clarifies. Leaving a minor leak whenever the user has a random id some way, some how. We could live with that, i guess 👍 i personally favor the separation by design though.

Still; get is a api method that does normalization and such. Using it internal seems overhead.

wouterj · 2016-08-20T13:18:22Z

Still; get is a user method that does normalization and such. Using it internal seems overhead.

The internal get() method skips almost all of the method. The first condition matches for new services and the method is simply called. (See https://github.com/symfony/dependency-injection/blob/master/Container.php#L258-L259 )

Leaving a minor leak whenever the user has a random id some way, some how.

We don't check for random ID (what's random?), we check if the ID exists in the array that contains generated IDs by the compiler.

ro0NL · 2016-08-20T13:26:51Z

Thanks @wouterj this was really clarifying, and probably makes a separate method obsolete.

What about if we do it right now? I really dont think we should bridge it with workarounds (no offense).

edit: about the leak; whenever you have $id = $this->privates[$id]; you can access a private service (what we're gonna do internally right? :-)). Anyway, doesnt matter they're not exposed, and nobody makes that good guesses.

sstok · 2016-08-20T14:25:58Z

The @trigger_error(..., E_USER_DEPRECATED); $id = $this->privates[$id]; is for BC only, in the future this resolving is simple removed, and as the original service-id is replaced with something random it will fail (saying the service doesn't exist).

ro0NL · 2016-08-20T14:55:47Z

@sstok @wouterj ping ping :-)

This works out pretty nice so far...

ogizanagi · 2016-08-20T15:26:22Z

@ro0NL: Even if your work looks great, @wouterj 's approach has the benefits of keeping things simple and pragmatic to fix the related issue.
I don't know if it's a good idea both trying to fix the issue and introducing the private services id generation into the very same PR as a "bug fix". It won't be a bug fix anymore, but somehow mixing bug fix, new feature and BC for this new feature. I'd say it's too much :/

But that's only my opinion, a core member is more legitimate to hint you on this. Just saying you should probably don't work too hard on this right now ;)

ro0NL · 2016-08-20T15:29:29Z

Im just playing around with randomizing now.. eventually i want to revert what i did to get. (The changes are not that dramatic).

However, the tests are updated to the right behavior. That was was an effort, yes.

I understand @wouterj 's approach and motivation, im only proposing a structural solution to consider as we can currently still target master.

ro0NL · 2016-08-20T16:47:29Z

@fabpot as this was your idea more or less.. WDYT?

Tests are failing nicely so far;

@@ @@

-        $instance->dep = $this->get('shared_private');
+        $instance->dep = $this->get('2a21409629880ccd9259eda073b930c1');

         return $instance;
     }

     /**
-     * Gets the 'shared_private' service.
+     * Gets the '2a21409629880ccd9259eda073b930c1' service.

@@ @@
      */
-    protected function getSharedPrivateService()
+    protected function get2a21409629880ccd9259eda073b930c1Service()
     {
-        return $this->services['shared_private'] = new \stdClass();
+        return $this->services['2a21409629880ccd9259eda073b930c1'] = new \stdClass();
     }

We have the map now in Container.. so we can really be flexible (ie. only use the random id for code, not docs).

sstok · 2016-08-21T07:19:03Z

src/Symfony/Component/DependencyInjection/Compiler/RandomizePrivateServiceIdentifiers.php

+        $this->idMap = array();
+        foreach ($container->getDefinitions() as $id => $definition) {
+            if (!$definition->isPublic()) {
+                $this->idMap[$id] = $this->randomizer ? (string) call_user_func($this->randomizer, $id) : md5(uniqid($id));


uniqid should not be used here, it's anything but unique and actually causes collisions very easily.

See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php#L291 for how it's done there.

sstok · 2016-08-21T07:41:53Z

Status: Needs work

ro0NL · 2016-08-21T10:14:31Z

@sstok would it make sense to mark a random definition (old private) as public after compiling? Currently we deprecated has etc. for all privates (random or not), but in get we must allow for it (if requested by the random id).

This doesnt make sense to me: has('random') // false vs. get('random') // object. Marking them public simplifies things :)

nicolas-grekas · 2016-08-22T17:36:56Z

We might require this kind of randomization on 4.0 when getting private service will be removed.
For now, I don't think it provides more external side effect than #19708.
If I'm not wrong, this could be closed in favor of #19708
WDYT?

stof · 2016-08-22T17:42:14Z

Yeah, I don't think we need to do this in 3.x

ro0NL · 2016-08-22T17:52:10Z

I dont mind :) i just dont like how we work with privates right now. And i do think this is the cleanest solution eventually, as the container does not have to care about privates. Currently leading to all sorts of quirkiness, as you might have noticed.

But yes, the pass can be added on top of #19708 in 4.x. From the component pov i would prefer this though (understanding the scope is wider, however we do advertise with standalone components ;-)).

nicolas-grekas · 2016-08-23T06:43:08Z

src/Symfony/Component/DependencyInjection/Compiler/RandomizePrivateServiceIdentifiers.php

+ *
+ * @author Roland Franssen <franssen.roland@gmail.com>
+ */
+class RandomizePrivateServiceIdentifiers implements CompilerPassInterface


IMO, randomization shouldn't be handled by a compiler pass, but by the PhpDumper directly. Otherwise it won't be applied without the compiler, and many apps won't have it enabled when migrating to 4.0

I considered this. But eventually chose to separate it into a compiler pass (yes; making it an optional feature). Without compiling you have a normal container.

Thing is i dont really like the concept of coupliing privates with a container. The container shouldnt care about privates (it's just a normal service from that perspective). Hence $this->privates could be totally unneeded...

No need to bypass get or checking private definitions in PhpDumper. We work with the container :)

What you call a normal container doesn't have private services. The container shouldn't care about private services, right, but the PhpDumper should (and already does). So I still think that the PhpDumper should do this.

We disagree :) i think we have compiler passes for that (optional) or otherwise Container::compile (by design).

A dumper (eg. PhpDumper) dumps the given container as is. Like others do, YAML, XML etc. I dont know why PhpDumer would randomize your services, whereas XmlDumper doesnt.

Currently it only deals with privates to hack the dumped container (ie. output), saying; here's a list of privates for you to validate business logic (which it shouldnt care about :)).

Because it makes no sense to randomize with XmlDumper and in fact would be wrong: it would rename services for no reason and make dumps harder and not accurate.
What matters is having dumpers deal with private services, which XmlDumper does by adding the public="false" attribute, and PhpDumper doing some randomization. That! would make sense.

Reconsidered :)

Because it makes no sense to randomize with XmlDumper and in fact would be wrong: it would rename services for no reason and make dumps harder and not accurate.

My first thought was, yeah this totally counts for a PhpDumper as well. Which in some ways is still true, but i guess you're right. Randomizing could be the way a php dumper deals with privates by design (whereas xml adds public="false" 👍 ).

Do we need to consider DX here?

Make it depend on some debug flag?

Have 2 implementations (think DebugPhpDumper)

~~Forget about randomizing, and compile different method names (think getPrivate(.+)Service)~~

Nah.. this makes the Container work with "privates" again 😕

Not sure how DX is involved here. So maybe but I don't what it'd mean :)

Forget about randomizing, and compile different method names

maybe :) the goal is to have private services be hidden from the outside, no interference with the public interface.
How it's done is an implementation detail We should chose the simplest one... :)

We should chose the simplest one

True.

DX, in the sense that you may not want to have random id's in the output, or at least customize/mock it in some way (think setRandomizer).

Then maybe the simplest and most DX friendly is to make private services leak a bit by reserving their id, throwing an exception whenever one uses or fetches an id that collides with a private service.

@wouterj

…ces internally (nicolas-grekas) This PR was merged into the 3.2-dev branch. Discussion ---------- [DI] Dont use Container::get() when fetching private services internally | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19683, #19682, #19680 | License | MIT As spotted by @wouterj, we forgot to remove the deprecation notice when doing internal calls to get private services. Yet, we don't need to get through this `get()` method, because we can already resolve many things at compile time for private services. This will provide another small performance optim, and fix the issue. Commits ------- a9c79fb [DI] Dont use Container::get() when fetching private services internally

nicolas-grekas · 2016-09-06T09:08:57Z

So, following the discussion above, my stand on this is:

Then maybe the simplest and most DX friendly is to make private services leak a bit by reserving their id, throwing an exception whenever one uses or fetches an id that collides with a private service.

This means not randomizing private services, even in 4.0.
Given that 4.0 is far away, I think we should close this PR, at least for now.
It's way too early to work on it and the issue will naturally arise when working on 3.4/4.0 when we'll work on cleaning deprecations.

ro0NL · 2016-09-07T07:37:20Z

👍 eventually it comes down to track private id's yes/no. Like currently..

Personally i'd favor randomizing id's in the PhpDumper.. (privates are just normal services).

But it doesnt matter much i guess.

carsonbot added Status: Needs Review Bug labels Aug 20, 2016

ro0NL changed the title ~~Di/dump privates~~ [DI] Fix sharing privates Aug 20, 2016

ro0NL reviewed Aug 20, 2016
View reviewed changes

ro0NL changed the title ~~[DI] Fix sharing privates~~ [WIP][DI] Fix sharing privates Aug 20, 2016

ro0NL changed the title ~~[WIP][DI] Fix sharing privates~~ [WIP][DI] Randomize private services Aug 20, 2016

ro0NL added 2 commits August 20, 2016 19:04

poc randomizing private services

286536f

fixed tests

bc3d636

sstok reviewed Aug 21, 2016
View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Aug 21, 2016

ro0NL added 3 commits August 21, 2016 12:46

refactored

1313bc1

try public randomized definitions

2cc36db

guarantee BC

e62470e

ro0NL changed the title ~~[WIP][DI] Randomize private services~~ [DI] Randomize private services Aug 21, 2016

nicolas-grekas mentioned this pull request Aug 22, 2016

[DI] Dont use Container::get() when fetching private services internally #19708

Merged

ro0NL mentioned this pull request Aug 22, 2016

[DI] Exclude privates from getServiceIds #19707

Closed

nicolas-grekas reviewed Aug 23, 2016
View reviewed changes

ro0NL mentioned this pull request Aug 23, 2016

[DI] Deprecate Container::set for nulls, initialized and alias services #19668

Closed

ro0NL closed this Sep 7, 2016

ro0NL deleted the di/dump-privates branch September 24, 2016 13:46

javiereguiluz mentioned this pull request May 17, 2018

[DI] Public, private and hidden services #27293

Closed

[DI] Randomize private services #19683

[DI] Randomize private services #19683

Conversation

ro0NL commented Aug 20, 2016 • edited Loading

ro0NL Aug 20, 2016 • edited Loading

Choose a reason for hiding this comment

wouterj commented Aug 20, 2016

ro0NL commented Aug 20, 2016 • edited Loading

ro0NL commented Aug 20, 2016

sstok commented Aug 20, 2016 • edited Loading

wouterj commented Aug 20, 2016

ro0NL commented Aug 20, 2016 • edited Loading

wouterj commented Aug 20, 2016

sstok commented Aug 20, 2016

ro0NL commented Aug 20, 2016 • edited Loading

wouterj commented Aug 20, 2016 • edited Loading

ro0NL commented Aug 20, 2016 • edited Loading

wouterj commented Aug 20, 2016

ro0NL commented Aug 20, 2016 • edited Loading

sstok commented Aug 20, 2016 • edited Loading

ro0NL commented Aug 20, 2016

ogizanagi commented Aug 20, 2016 • edited Loading

ro0NL commented Aug 20, 2016 • edited Loading

ro0NL commented Aug 20, 2016

sstok Aug 21, 2016

Choose a reason for hiding this comment

sstok commented Aug 21, 2016

ro0NL commented Aug 21, 2016 • edited Loading

nicolas-grekas commented Aug 22, 2016

stof commented Aug 22, 2016

ro0NL commented Aug 22, 2016 • edited Loading

nicolas-grekas Aug 23, 2016

Choose a reason for hiding this comment

ro0NL Aug 23, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Aug 24, 2016

Choose a reason for hiding this comment

ro0NL Aug 24, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Aug 24, 2016 • edited Loading

Choose a reason for hiding this comment

ro0NL Aug 24, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Aug 24, 2016

Choose a reason for hiding this comment

ro0NL Aug 24, 2016

Choose a reason for hiding this comment

nicolas-grekas Aug 24, 2016

Choose a reason for hiding this comment

nicolas-grekas commented Sep 6, 2016

ro0NL commented Sep 7, 2016

ro0NL commented Aug 20, 2016 •

edited

Loading

ro0NL Aug 20, 2016 •

edited

Loading

ro0NL commented Aug 20, 2016 •

edited

Loading

sstok commented Aug 20, 2016 •

edited

Loading

ro0NL commented Aug 20, 2016 •

edited

Loading

ro0NL commented Aug 20, 2016 •

edited

Loading

wouterj commented Aug 20, 2016 •

edited

Loading

ro0NL commented Aug 20, 2016 •

edited

Loading

ro0NL commented Aug 20, 2016 •

edited

Loading

sstok commented Aug 20, 2016 •

edited

Loading

ogizanagi commented Aug 20, 2016 •

edited

Loading

ro0NL commented Aug 20, 2016 •

edited

Loading

ro0NL commented Aug 21, 2016 •

edited

Loading

ro0NL commented Aug 22, 2016 •

edited

Loading

ro0NL Aug 23, 2016 •

edited

Loading

ro0NL Aug 24, 2016 •

edited

Loading

nicolas-grekas Aug 24, 2016 •

edited

Loading

ro0NL Aug 24, 2016 •

edited

Loading