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

Deterministic proxy names #25978

Merged
merged 1 commit into from Jan 31, 2018

Conversation

@lstrojny
Copy link
Contributor

commented Jan 30, 2018

Proxy class names should be deterministic and independent of spl_object_hash() which is somewhat random to better support reproducible builds. See #25958

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -
@@ -108,7 +108,7 @@ public function getProxyCode(Definition $definition)
*/
private function getProxyClassName(Definition $definition)
{
return preg_replace('/^.*\\\\/', '', $definition->getClass()).'_'.substr(hash('sha256', spl_object_hash($definition).$this->salt), -7);
return preg_replace('/^.*\\\\/', '', $definition->getClass()).'_'.substr(hash('sha256', serialize($definition).$this->salt), -7);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 30, 2018

Member

I just looked at the current logic. It looks like this would be enough isn't it? (with lazy prefix to easy recognizing them):
return 'lazy'.preg_replace('/^.*\\\\/', '', $definition->getClass()).'_'.substr(hash('sha256', $definition->getClass()), -7);

This comment has been minimized.

Copy link
@lstrojny

lstrojny Jan 30, 2018

Author Contributor

Indeed, that should be enough. The only scenario where this could break is if we would have the same class name with a different shape in one process. That is probably achievable through runkit and the like but I guess that’s a case we can ignore.

…ct_hash() which is somewhat random
@lstrojny lstrojny force-pushed the lstrojny:dev/deterministic-proxy-class-names branch from 50b1319 to b173b81 Jan 30, 2018
@lstrojny

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

@nicolas-grekas not sure what to do about the resource tracking topic. Probably worth another PR?

@stof

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

@lstrojny we currently don't invalidate the cached container when the class changes, meaning the generated proxy class might not be regenerated properly when needed.
Adding the method call suggested by @nicolas-grekas will register the appropriate resource tracking.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

I fixed resource tracking in #25981

@stof
stof approved these changes Jan 31, 2018
@@ -108,7 +108,7 @@ public function getProxyCode(Definition $definition)
*/
private function getProxyClassName(Definition $definition)
{
return preg_replace('/^.*\\\\/', '', $definition->getClass()).'_'.substr(hash('sha256', spl_object_hash($definition).$this->salt), -7);
return preg_replace('/^.*\\\\/', '', $definition->getClass()).'_'.substr(hash('sha256', $definition->getClass().$this->salt), -7);

This comment has been minimized.

Copy link
@sroze

sroze Jan 31, 2018

Member

Does this mean we only have a maximum of one proxy per class? 🤔

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jan 31, 2018

Member

does it mean anything to have more that one proxy per class? do you have an example?

This comment has been minimized.

Copy link
@stof

stof Jan 31, 2018

Member

you will get only one proxy class per class. You can still have multiple instance.

If you look at the code, the only part of the Definition impact the generated class is the class name. So multiple definitions can reuse the same proxy class safely.

This comment has been minimized.

Copy link
@sroze

sroze Jan 31, 2018

Member

You are right, this makes sense :)

@fabpot
fabpot approved these changes Jan 31, 2018
@fabpot

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

Thank you @lstrojny.

@fabpot fabpot merged commit b173b81 into symfony:3.4 Jan 31, 2018
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
fabpot added a commit that referenced this pull request Jan 31, 2018
This PR was merged into the 3.4 branch.

Discussion
----------

Deterministic proxy names

Proxy class names should be deterministic and independent of spl_object_hash() which is somewhat random to better support reproducible builds. See #25958

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Commits
-------

b173b81 Proxy class names should be deterministic and independent of spl_object_hash() which is somewhat random
@lstrojny lstrojny deleted the lstrojny:dev/deterministic-proxy-class-names branch Jan 31, 2018
This was referenced Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.