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

[VarDumper] add a GMP caster in order to cast GMP resources into string or integer #25237

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
6 participants
@Simperfit
Contributor

Simperfit commented Dec 1, 2017

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 todo

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:
img_2844

*/
class GmpCaster
{
public static function convertToInt(\GMP $value): int

This comment has been minimized.

@stof

stof Dec 1, 2017

Member

this is not what a VarDumper caster is.

@@ -29,7 +29,8 @@
},
"suggest": {
"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"

This comment has been minimized.

@stof

stof Dec 1, 2017

Member

this does not make any sense: the VarDumper component does not provide any feature needing gmp. It would just improve the dumping when your own code uses gmp. So there is no point suggesting it. It would just be spam in the suggestions

This comment has been minimized.

@Simperfit

Simperfit Dec 1, 2017

Contributor

i'll remove it

.travis.yml Outdated
@@ -97,6 +97,7 @@ before_install:
echo extension = redis.so >> $INI
echo extension = memcached.so >> $INI
echo extension = mongodb.so >> $INI
echo extension = gmp.so >> $INI

This comment has been minimized.

@stof

stof Dec 1, 2017

Member

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

This comment has been minimized.

@Simperfit

Simperfit Dec 1, 2017

Contributor

i'll remove it too

@sroze

This comment has been minimized.

Member

sroze commented Dec 1, 2017

It's clearly a WIP PR. @Simperfit I suggest you either add WIP to your PR title or just not open the PR yet (and enable Travis on your fork if you need the tests to run) :)

@Simperfit Simperfit changed the title from [VarDumper] add a GMP caster in order to cast GMP resources into string or integer to [VarDumper] [WIP] add a GMP caster in order to cast GMP resources into string or integer Dec 1, 2017

*/
class GmpCaster
{
public static function castInt(\GMP $value): array

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

see other casters: that not their typical signature, we should use the same here also

public static function castString(\GMP $value): array
{
return array('value' => gmp_strval($value));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

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.

@@ -112,6 +112,8 @@
'DateTimeZone' => array('Symfony\Component\VarDumper\Caster\DateCaster', 'castTimeZone'),
'DatePeriod' => array('Symfony\Component\VarDumper\Caster\DateCaster', 'castPeriod'),
'GMP' => array('Symfony\Component\VarDumper\Caster\GmpCaster', 'castInt'),

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

not all GMP object's values can be represented as strings, so castString should be the implementation

@@ -126,6 +128,9 @@
':persistent stream' => array('Symfony\Component\VarDumper\Caster\ResourceCaster', 'castStream'),
':stream-context' => array('Symfony\Component\VarDumper\Caster\ResourceCaster', 'castStreamContext'),
':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'),

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

GMP as resources cannot happen anymore on PHP 7.1, this should be removed.

return array('value' => gmp_intval($value));
}
public static function castString(\GMP $value): array

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

this should be renamed "castGmp"

{
public function testCastInt()
{
if (false === extension_loaded('gmp')) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

@requires extension gmp instead

EOTXT;
$this->assertStringMatchesFormat($expected, print_r($clone, true));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

let's use assertDumpEquals instead

@Simperfit Simperfit changed the title from [VarDumper] [WIP] add a GMP caster in order to cast GMP resources into string or integer to [VarDumper] add a GMP caster in order to cast GMP resources into string or integer Dec 1, 2017

"""
EODUMP;
$this->assertDumpEquals($expected, print_r($clone, true));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

this is playing around assertDumpEquals
just $this->assertDumpEquals($expected, gmp_init('0101')); instead

/**
* @requires extension gmp
*/
public function testGmpCast()

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

but in fact, this should not be in VarClonerTest
the existing test in the caster in enough IMO

/**
* @requires extension gmp
*/
public function testCastString()

This comment has been minimized.

@nicolas-grekas
{
public static function castGmp(\GMP $gmp, array $a, Stub $stub, $isNested, $filter): array
{
$a[Caster::PREFIX_VIRTUAL.'gmpString'] = (string) new ConstStub(\GMP::class, gmp_strval($gmp));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

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));

use Symfony\Component\VarDumper\Cloner\Stub;
/**
* Transform a GMP object to a integer or a string. It need the gmp php extension.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2017

Member

* Casts GMP objects to array representation.

@Simperfit

This comment has been minimized.

Contributor

Simperfit commented Dec 1, 2017

here is a little gif of the french fields with snow:
image uploaded from ios

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Dec 1, 2017

@fabpot

fabpot approved these changes Dec 1, 2017

@fabpot

This comment has been minimized.

Member

fabpot commented Dec 1, 2017

Thank you @Simperfit.

@fabpot fabpot merged commit ed2c1af into symfony:master Dec 1, 2017

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

fabpot added a commit that referenced this pull request Dec 1, 2017

feature #25237 [VarDumper] add a GMP caster in order to cast GMP reso…
…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        | todo

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:
![img_2844](https://user-images.githubusercontent.com/3451634/33472917-8b48913e-d674-11e7-923f-ad951f7f2966.JPG)

Commits
-------

ed2c1af [VarDumper] add a GMP caster in order to cast GMP resources into string or integer

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@Simperfit Simperfit deleted the Simperfit:feature/add-a-caster-for-gmp-numbers branch May 30, 2018

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