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

Cast result to int before adding to it #20539

Merged
merged 1 commit into from Dec 8, 2016

Conversation

Projects
None yet
6 participants
@alcaeus
Contributor

alcaeus commented Nov 16, 2016

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.

@xabbuh

This comment has been minimized.

Member

xabbuh commented Nov 16, 2016

How can this actually happen?

@alcaeus

This comment has been minimized.

Contributor

alcaeus commented Nov 16, 2016

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.

@xabbuh

This comment has been minimized.

Member

xabbuh commented Nov 16, 2016

Hm, indeed. Guess we should deprecate that for 4.0.

@alcaeus

This comment has been minimized.

Contributor

alcaeus commented Nov 17, 2016

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?

venyii added a commit to venyii/Sylius that referenced this pull request Nov 17, 2016

[Taxation] Remove useless argument type property
This fixes a 'non-numeric value encountered' warning when running
PHP7.1. PR in Symfony: symfony/symfony#20539
@xabbuh

This comment has been minimized.

Member

xabbuh commented Nov 17, 2016

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?

@alcaeus alcaeus changed the base branch from 2.8 to 2.7 Nov 17, 2016

@alcaeus

This comment has been minimized.

Contributor

alcaeus commented Nov 17, 2016

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.

@xabbuh

This comment has been minimized.

Member

xabbuh commented Nov 17, 2016

Rebasing and force pushing again may help Travis.

@stof

This comment has been minimized.

Member

stof commented Nov 17, 2016

yeah, this happens becomes Sylius mixes integer (implicit) keys and associative keys.
This would still not work properly anyway, as a non-numeric key should just be ignored when searching the max. So the bug fix is incomplete.
And we should indeed trigger a deprecation in Symfony 3.3 when mixing this IMO (but with the proper detection logic).

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 <argument key="..."> only makes sense inside an array argument (<argument type="collection">). So I suggest you to also clean the Sylius service definition

@alcaeus

This comment has been minimized.

Contributor

alcaeus commented Nov 17, 2016

So I suggest you to also clean the Sylius service definition

Sylius/Sylius#6752 was merged, so it's clean in dev-master.

According to the schema, it is valid to have key in an argument even when it's outside of a collection. I agree that it makes no sense and that it should be deprecated.

As for ignoring numeric keys, I could do that, which would change the resulting array (instead of having ['type' => 'bar', 1 => foo] it would be ['type' => 'bar', 0 => foo]). If that's acceptable, I'll gladly filter out non-numeric values before passing it to max.

@stof

This comment has been minimized.

Member

stof commented Nov 17, 2016

@alcaeus this is because the XSD schema does not make a distinction between <argument> used inside <service> or inside another <argument> (this was done this way to avoid making the XSD more complex, but it might have been a mistake).

@alcaeus

This comment has been minimized.

Contributor

alcaeus commented Nov 17, 2016

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.

pjedrzejewski pushed a commit to Sylius/SyliusCoreBundle that referenced this pull request Nov 17, 2016

[Taxation] Remove useless argument type property
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;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 25, 2016

Member

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')) {
Cast result to int before adding to it
This fixes the occasional warning about non-numeric values when using PHP 7.1
@alcaeus

This comment has been minimized.

Contributor

alcaeus commented Nov 25, 2016

@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 array_keys and array_pop. Relying on a loop just leaving variables around just feels dirty.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Dec 6, 2016

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 6, 2016

👍
status: reviewed

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Dec 8, 2016

Thank you @alcaeus.

@nicolas-grekas nicolas-grekas merged commit 70c42f2 into symfony:2.7 Dec 8, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 8, 2016

bug #20539 Cast result to int before adding to it (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

@alcaeus alcaeus deleted the alcaeus:fix-max-php71 branch Apr 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment