Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[VarDumper] add a GMP caster in order to cast GMP resources into string or integer#25237
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[VarDumper] add a GMP caster in order to cast GMP resources into string or integer#25237
Uh oh!
There was an error while loading.Please reload this page.
Conversation
75d9636 tod4c7ce7Compare| */ | ||
| class GmpCaster | ||
| { | ||
| publicstaticfunctionconvertToInt(\GMP$value):int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is not what a VarDumper caster is.
| "ext-iconv":"To convert non-UTF-8 strings to UTF-8 (or symfony/polyfill-iconv in case ext-iconv cannot be used).", | ||
| "ext-intl":"To show region name in time zone dump" | ||
| "ext-intl":"To show region name in time zone dump", | ||
| "ext-gmp":"To dump GMP resources to string or integer" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this does not make any sense: the VarDumper component does not provide any featureneeding gmp. It would just improve the dumping whenyour own code uses gmp. So there is no point suggesting it. It would just be spam in the suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i'll remove it
.travis.yml Outdated
| echo extension = redis.so >> $INI | ||
| echo extension = memcached.so >> $INI | ||
| echo extension = mongodb.so >> $INI | ||
| echo extension = gmp.so >> $INI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
there is no gmp.so on Travis. So this is breaking the CI.
gmp is already available on Travis, as it is built as a bundled extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i'll remove it too
sroze commentedDec 1, 2017
It's clearly a WIP PR.@Simperfit I suggest you either add |
2a4413b todd04bd0Compare| */ | ||
| class GmpCaster | ||
| { | ||
| publicstaticfunctioncastInt(\GMP$value):array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
see other casters: that not their typical signature, we should use the same here also
| publicstaticfunctioncastString(\GMP$value):array | ||
| { | ||
| returnarray('value' =>gmp_strval($value)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This means "value" will be displayed as if it were a public property on the object, which is wrong
should be a virtual property (see other casters)
Also: this will display the value as if it were a string, which is also a bad hint. I suggest to use a ConstStub to wrap the value, to that it is displayed in a more specific way. I invite you to compare with/without and decide.
| 'DateTimeZone' =>array('Symfony\Component\VarDumper\Caster\DateCaster','castTimeZone'), | ||
| 'DatePeriod' =>array('Symfony\Component\VarDumper\Caster\DateCaster','castPeriod'), | ||
| 'GMP' =>array('Symfony\Component\VarDumper\Caster\GmpCaster','castInt'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
not all GMP object's values can be represented as strings, so castString should be the implementation
| ':xml' =>array('Symfony\Component\VarDumper\Caster\XmlResourceCaster','castXml'), | ||
| ':gmp' =>array('Symfony\Component\VarDumper\Caster\GmpCaster','castInt'), | ||
| ':gmp int' =>array('Symfony\Component\VarDumper\Caster\GmpCaster','castInt'), | ||
| ':gmp string' =>array('Symfony\Component\VarDumper\Caster\GmpCaster','castString'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
GMP as resources cannot happen anymore on PHP 7.1, this should be removed.
| returnarray('value' =>gmp_intval($value)); | ||
| } | ||
| publicstaticfunctioncastString(\GMP$value):array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this should be renamed "castGmp"
| { | ||
| publicfunctiontestCastInt() | ||
| { | ||
| if (false ===extension_loaded('gmp')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@requires extension gmp instead
| EOTXT; | ||
| $this->assertStringMatchesFormat($expected,print_r($clone,true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
let's useassertDumpEquals instead
dd04bd0 to452a273Compare452a273 to516e25dCompare| """ | ||
| EODUMP; | ||
| $this->assertDumpEquals($expected,print_r($clone,true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this is playing around assertDumpEquals
just$this->assertDumpEquals($expected, gmp_init('0101')); instead
516e25d to5f1f345Compare| /** | ||
| * @requires extension gmp | ||
| */ | ||
| publicfunctiontestGmpCast() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
but in fact, this should not be in VarClonerTest
the existing test in the caster in enough IMO
| /** | ||
| * @requires extension gmp | ||
| */ | ||
| publicfunctiontestCastString() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
bad name
| { | ||
| publicstaticfunctioncastGmp(\GMP$gmp,array$a,Stub$stub,$isNested,$filter):array | ||
| { | ||
| $a[Caster::PREFIX_VIRTUAL.'gmpString'] = (string)newConstStub(\GMP::class,gmp_strval($gmp)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
s/gmpString/value/
the what's the prupose of the cast to string? looks wrong to me
should be:$a[Caster::PREFIX_VIRTUAL.'num'] = new ConstStub(gmp_strval($gmp), gmp_strval($gmp));
| useSymfony\Component\VarDumper\Cloner\Stub; | ||
| /** | ||
| * Transform a GMP object to a integer or a string. It need the gmp php extension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
* Casts GMP objects to array representation.
369e481 to530d71bCompareSimperfit commentedDec 1, 2017
here is a little gif of the french fields with snow: |
530d71b to4c7a3f7Compare4c7a3f7 toed2c1afComparefabpot commentedDec 1, 2017
Thank you@Simperfit. |
…urces into string or integer (Simperfit)This PR was merged into the 4.1-dev branch.Discussion----------[VarDumper] add a GMP caster in order to cast GMP resources into string or integer| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#25222| License | MIT| Doc PR | todoDo you want to dump that kind of resources ? Then it means that the app you are writing is doing some math... why?! :pIt quite nice the little snow that we have in the north of france right now:Commits-------ed2c1af [VarDumper] add a GMP caster in order to cast GMP resources into string or integer
Do you want to dump that kind of resources ? Then it means that the app you are writing is doing some math... why?! :p
It quite nice the little snow that we have in the north of france right now:
