8000 [DependencyInjection] Fix duplication of placeholders by mickadoo · Pull Request #20199 · symfony/symfony · GitHub
[go: up one dir, main page]
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.

Sign up for GitHub

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
Closed

[DependencyInjection] Fix duplication of placeholders #20199

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1e9d875
[DependencyInjection] Fix duplication of placeholders when merging En…
mickadoo Oct 11, 2016
a0373ba
[DependencyInjection] Changes from code review in how placeholders ar…
mickadoo Oct 11, 2016
92da498
[DependencyInjection] Changes from code review to revert unnecessary …
mickadoo Oct 11, 2016
574da52
[DependencyInjection] Changes from code review to use assertCount ins…
mickadoo Oct 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[DependencyInjection] Fix duplication of placeholders when merging En… 
…vPlaceholderParameterBags
  • Loading branch information
@mickadoo
mickadoo committed Oct 11, 2016
commit 1e9d87527fa56ea9fde94ab0c0f97d2f50f3e780
10 changes: 9 additions & 1 deletion src/Symfony/Component/DependencyInjection/ParameterBag/EnvPlaceholderParameterBag.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

*/
public function mergeEnvPlaceholders(self $bag)
{
$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 •
edited
Loading

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...

$existingEntries = isset($this->envPlaceholders[$key]) ? $this->envPlaceholders[$key] : array();
$mergedEntries = array_merge($existingEntries, $newEntries);
$this->envPlaceholders[$key] = array_unique($mergedEntries);
}
}
}
15 changes: 15 additions & 0 deletions ...mfony/Component/DependencyInjection/Tests/ParameterBag/EnvPlaceholderParameterBagTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,19 @@ public function testGetThrowsInvalidArgumentExceptionIfEnvNameContainsNonWordCha
$bag = new EnvPlaceholderParameterBag();
$bag->get('env(%foo%)');
}

public function testMergeWillNotDuplicateIdenticalParameters()
{
$originalPlaceholders = array('database_host' => array('localhost'));
$firstBag = new EnvPlaceholderParameterBag($originalPlaceholders);

// initialize placeholders
$firstBag->get('env(database_host)');
$secondBag = clone $firstBag;

$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()

}
}
0