Skip to content

[DependencyInjection] Fix duplication of placeholders #20199

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

Closed
wants to merge 4 commits into from

Conversation

mickadoo
Copy link
Contributor

@mickadoo mickadoo commented Oct 11, 2016

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

Fixes performance issues related to merging parameter bags when using the env() parameters introduced in #19681

@@ -62,9 +62,17 @@ public function getEnvPlaceholders()

/**
* Merges the env placeholders of another EnvPlaceholderParameterBag.
*
* @param EnvPlaceholderParameterBag $bag
Copy link
Member

Choose a reason for hiding this comment

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

should be remove as it doesn't add any value when considering the method signature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it because it looks like most of the docblocks for methods in the Symfony core either use {@inheritdoc} or an explicit description of parameters. You're right (and I'm sure the clean code principles would agree with you) that it doesn't add anything by looking at it, although inside an IDE like PHPStorm people will use the quick documentation feature which makes it a little bit more understandable what the type parameter type is, but of course I can remove it - just let me know

Copy link
Member

Choose a reason for hiding this comment

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

We generally prefer removing those, eventhough the code base is not always consistent on this aspect.

Copy link
Contributor Author

@mickadoo mickadoo Oct 11, 2016

Choose a reason for hiding this comment

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

Sure, I'll change it then. I'll wait a bit for your response to the other part before committing and pushing.

$this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders());
$newPlaceholders = $bag->getEnvPlaceholders();

foreach ($newPlaceholders as $key => $newEntries) {
Copy link
Member

Choose a reason for hiding this comment

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

What about this? should do the same isn't it?
$this->envPlaceholders = array_map('array_unique', array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders()));

Copy link
Member

Choose a reason for hiding this comment

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

or what about another approach like this?

@@ -30,7 +30,9 @@ class EnvPlaceholderParameterBag extends ParameterBag
             $env = substr($name, 4, -1);

             if (isset($this->envPlaceholders[$env])) {
-                return $this->envPlaceholders[$env][0];
+                foreach ($this->envPlaceholders[$env] as $placeholder) {
+                    return $placeholder;
+                }
             }
             if (preg_match('/\W/', $env)) {
                 throw new InvalidArgumentException(sprintf('Invalid %s name: only "word" characters are allowed.', $name));
@@ -43,8 +45,9 @@ class EnvPlaceholderParameterBag extends ParameterBag
                     throw new RuntimeException(sprintf('The default value of an env() parameter must be scalar, but "%s" given to "%s".', gettype($defaultValue), $name));
                 }
             }
+            $placeholder = sprintf('env_%s_%s', $env, md5($name.uniqid(mt_rand(), true)));

-            return $this->envPlaceholders[$env][] = sprintf('env_%s_%s', $env, md5($name.uniqid(mt_rand(), true)));
+            return $this->envPlaceholders[$env][$placeholder] = $placeholder;
         }

         return parent::get($name);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About the array_map, it works and it cuts some lines of code, but it is needless use of recursive functions and maybe not as clear, is it really needed?

For the changes inside get(), why would you change from returning the 0th index to using foreach? If you think this should be changed how about something more readable like

return array_values($this->envPlaceholders[$placeholder])[0];

Finally for the changes in setting $this->envPlaceholders I don't see the point in an array that will always have the structure

[
  'apple' => 'apple',
  'orange' => 'orange'
]

Plus (and I know it's going outside the scope of this PR) I don't think that assignment in a return statement is very clear and would prefer something like:

$uniqueName = md5($name.uniqid(mt_rand(), true));
$placeholder = sprintf('env_%s_%s', $environmentVariableName, $uniqueName);
$this->envPlaceholders[$environmentVariableName][] = $placeholder;

return $placeholder;

I made some naming changes locally because I also found env confusing as it conflicts with Symfony environments but these don't have to be included in any PR changes.

Sorry to sound like a dick and that I don't want to change it, but I think I should at least be sure about the changes before I make them. I think it's a really cool feature and the only reason I'm here is because when I was working on changing my own small project to use the SYMFONY__ENV variable style I came across this and thought it looks much better.

Copy link
Member

@nicolas-grekas nicolas-grekas Oct 11, 2016

Choose a reason for hiding this comment

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

needless use of recursive functions

not needless at all: the array is recursive (width two depth levels)

array_values($this->envPlaceholders[$placeholder])[0]

this would create a full copy of the entire array just to get the first element. foreach is the most efficient way to do so when not knowing the first key.

don't see the point in an array that will always have the structure

the point is using keys as a precomputed deduplication index, so that e.g. array_unique (slow compared to indexes) is not needed anymore.

I don't think that assignment in a return statement

no pb for that change, go for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see your point for most of those, but really using a 3 line foreach to get the first element of an array seems like overkill. How about

return reset($this->envPlaceholders[$env]); // return first result

Copy link
Member

Choose a reason for hiding this comment

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

reset has two drawbacks: it works by reference, which I prefer to avoid, and it changes the internal state of the array, which is a not wanted side effect. Even if these could be of no importance in this case, I consider using it a worse practice...

@@ -65,6 +71,9 @@ public function getEnvPlaceholders()
*/
public function mergeEnvPlaceholders(self $bag)
{
$this->envPlaceholders = array_merge_recursive($this->envPlaceholders, $bag->getEnvPlaceholders());
$this->envPlaceholders = array_map(
Copy link
Member

Choose a reason for hiding this comment

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

this change is not needed anymore: now that keys are strings, they are already unique

@nicolas-grekas
Copy link
Member

👍

$firstBag->mergeEnvPlaceholders($secondBag);
$mergedPlaceholders = $firstBag->getEnvPlaceholders();

$this->assertEquals(1, count($mergedPlaceholders['database_host']));
Copy link
Contributor

Choose a reason for hiding this comment

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

assertCount()

@@ -30,7 +30,9 @@ public function get($name)
$env = substr($name, 4, -1);

if (isset($this->envPlaceholders[$env])) {
return $this->envPlaceholders[$env][0];
foreach ($this->envPlaceholders[$env] as $placeholder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

current()?

Copy link
Member

Choose a reason for hiding this comment

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

see #20199 (comment), same applies to current

@fabpot
Copy link
Member

fabpot commented Oct 11, 2016

Thank you @mickadoo.

@fabpot fabpot closed this Oct 11, 2016
fabpot added a commit that referenced this pull request Oct 11, 2016
…kadoo)

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

Discussion
----------

[DependencyInjection] Fix duplication of placeholders

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

Fixes performance issues related to merging parameter bags when using the `env()` parameters introduced in #19681

Commits
-------

124f30d [DependencyInjection] Fix duplication of placeholders
@mickadoo mickadoo deleted the fix/env-placeholder-merging branch October 13, 2016 12:25
nicolas-grekas added a commit that referenced this pull request Oct 14, 2016
…adoo)

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

Discussion
----------

Fix/broken merging of parameter bag env placeholders

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

In PR #20199 I made changes after review that broke the use of env variables, sorry about that - should have checked it a bit more before making the changes.

@nicolas-grekas, I know you're very busy with all that merging, but if you could take a look at this it would be great since you know most about it.

Commits
-------

b53e0de Fix/broken merging of parameter bag env placeholders
@fabpot fabpot mentioned this pull request Oct 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants