Cast result to int before adding to it #20539
Conversation
|
How can this actually happen? |
|
I've had it happen when loading this service definition: https://github.com/Sylius/Sylius/blob/82815edc768459159b4ea5910100d6e3f50bb8b3/src/Sylius/Bundle/CoreBundle/Resources/config/services/taxation.xml#L33..L40 Note: I'll add that as a test case, just wanted to get opinions before spending the time doing that. According to the schema, it is a valid service definition, albeit an unusual one. |
|
Hm, indeed. Guess we should deprecate that for 4.0. |
e893708 to
ac6c865
Compare
|
Ok, I've added a test for the specific use case. Should I add the deprecation here or do you want to discuss that first? |
|
As deprecations always must be done in the next minor version, but this is a bug fix that should be merged into all maintained branches, we should do the deprecation in a different PR. By the way, isn't Symfony 2.7 affected by this too? |
ac6c865 to
55871d0
Compare
|
You are correct - I merely forgot that it was a LTS release as well. I changed the base to 2.7, but travis-ci seems to have missed it. |
|
Rebasing and force pushing again may help Travis. |
55871d0 to
9f9e8ab
Compare
|
yeah, this happens becomes Sylius mixes integer (implicit) keys and associative keys. Btw, it is useless in the case of Sylius anyway, as top-level arguments don't care about the keys (as PHP does not have named arguments). Using |
Sylius/Sylius#6752 was merged, so it's clean in dev-master. According to the schema, it is valid to have As for ignoring numeric keys, I could do that, which would change the resulting array (instead of having |
|
@alcaeus this is because the XSD schema does not make a distinction between |
|
I guessed as much. Once keys outside of collection arguments have been forbidden (so with 4.0) the schema should be updated to reflect this. |
This fixes a 'non-numeric value encountered' warning when running PHP7.1. PR in Symfony: symfony/symfony#20539
| @@ -347,7 +347,7 @@ private function getArgumentsAsPhp(\DOMElement $node, $name, $lowercase = true) | |||
| } | |||
|
|
|||
| if (!$arg->hasAttribute('key')) { | |||
| $key = !$arguments ? 0 : max(array_keys($arguments)) + 1; | |||
| $key = !$arguments ? 0 : (int) max(array_keys($arguments)) + 1; | |||
There was a problem hiding this comment.
The intend here is to compute the next free integer index. This cast might hide a wrong decision.
I propose this patch instead:
--- a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
+++ b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
@@ -346,21 +346,22 @@ class XmlFileLoader extends FileLoader
$arg->setAttribute('key', $arg->getAttribute('name'));
}
- if (!$arg->hasAttribute('key')) {
- $key = !$arguments ? 0 : max(array_keys($arguments)) + 1;
- } else {
- $key = $arg->getAttribute('key');
- }
-
- // parameter keys are case insensitive
- if ('parameter' == $name && $lowercase) {
- $key = strtolower($key);
- }
-
// this is used by DefinitionDecorator to overwrite a specific
// argument of the parent definition
if ($arg->hasAttribute('index')) {
$key = 'index_'.$arg->getAttribute('index');
+ } elseif (!$arg->hasAttribute('key')) {
+ $arguments[] = null;
+ // compute the just added last key in $arguments
+ foreach ($arguments as $key => $value) {
+ }
+ } else {
+ $key = $arg->getAttribute('key');
+
+ // parameter keys are case insensitive
+ if ('parameter' == $name && $lowercase) {
+ $key = strtolower($key);
+ }
}
switch ($arg->getAttribute('type')) {This fixes the occasional warning about non-numeric values when using PHP 7.1
9f9e8ab to
70c42f2
Compare
|
@nicolas-grekas good idea! I changed the code a little bit, opting for a more explicit approach to fetch the last array key by using |
|
👍 |
|
Thank you @alcaeus. |
This PR was merged into the 2.7 branch. Discussion ---------- Cast result to int before adding to it | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | This fixes the occasional warning about non-numeric values when using PHP 7.1. Commits ------- 70c42f2 Cast result to int before adding to it
This fixes the occasional warning about non-numeric values when using PHP 7.1.