[VarDumper] Improve dump of AMQP* Object #21557

Merged
merged 1 commit into from Feb 11, 2017

Projects

None yet

4 participants

@lyrixx
Member
lyrixx commented Feb 7, 2017 edited
Q A
Branch? 2.8
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

The release of https://github.com/pdezwart/php-amqp/ 1.7.0alpha1
changed internally the handling of AMQP* object. So now when dumping
using var_dump(), many information are available. So many information
are displayed twice. This commit fixes this issue and keeps displaying basic
information for older versions of the lib.

Reference:


screenshot4

screenshot5

@stof
Member
stof commented Feb 7, 2017

shouldn't we rather avoid all the useless info instead of overwriting the existing ones ?

@nicolas-grekas nicolas-grekas added this to the 2.8 milestone Feb 7, 2017
@lyrixx
Member
lyrixx commented Feb 8, 2017

shouldn't we rather avoid all the useless info instead of overwriting the existing ones ?

The things is: it's hard to know when property X or property Y has been added in the extension. So If we want to avoid overwriting, we should check for each property if it has been defined before.

WDYT?

@stof
Member
stof commented Feb 8, 2017

Well, you are switching the prefix globally anyway, which implies that they all changed together.

@lyrixx
Member
lyrixx commented Feb 8, 2017

Indeed. But anyway $array += [] does not overwrite the initial value. So IMHO, it's not an issue. I just specifically, overwrite some properties, to enchance the display.

But just to be sure I understand, you propose something like that:


        // Recent version of the extension already expose internal properties
        if (isset($a["\x00AMQPConnection\x00login"])) {
            $a += [
                Caster::PREFIX_VIRTUAL.'isConnected' => $c->isConnected(),
            ];

            return $a;
        }

        $prefix = Caster::PREFIX_VIRTUAL;

        // BC layer in the amqp lib
        if (method_exists($c, 'getReadTimeout')) {
            $timeout = $c->getReadTimeout();
        } else {
            $timeout = $c->getTimeout();
        }

        $a += array(
            $prefix.'isConnected' => $c->isConnected(),
            $prefix.'login' => $c->getLogin(),
            $prefix.'password' => $c->getPassword(),
            $prefix.'host' => $c->getHost(),
            $prefix.'vhost' => $c->getVhost(),
            $prefix.'port' => $c->getPort(),
            $prefix.'read_timeout' => $timeout,
        );

        return $a;
@lyrixx
Member
lyrixx commented Feb 8, 2017

@stof I updated my PR according to your comments.

@lyrixx lyrixx [VarDumper] Improve dump of AMQP* Object
The release of https://github.com/pdezwart/php-amqp/ 1.7.0alpha1
changed internally the handlings of AMQP* object. So now when dumping
using var_dump(), many information are availables. So many information
are displayed twice. This commit fixes this issue and keeps displaying basic
information for older version of the lib.

Reference:

* https://pecl.php.net/package-info.php?package=amqp&version=1.7.0alpha1
* pdezwart/php-amqp@314afbc (and next commits)
c553352
@stof
Member
stof commented Feb 8, 2017

๐Ÿ‘

Btw, don't we need to update some tests ?

@lyrixx
Member
lyrixx commented Feb 8, 2017

Btw, don't we need to update some tests ?

This part is not tested... :/

@nicolas-grekas
Member

Thank you @lyrixx.

@nicolas-grekas nicolas-grekas merged commit c553352 into symfony:2.8 Feb 11, 2017

2 of 3 checks passed

fabbot.io [braces] Is not configurable.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolas-grekas nicolas-grekas added a commit that referenced this pull request Feb 11, 2017
@nicolas-grekas nicolas-grekas bug #21557 [VarDumper] Improve dump of AMQP* Object (lyrixx)
This PR was merged into the 2.8 branch.

Discussion
----------

[VarDumper] Improve dump of AMQP* Object

| Q             | A
| ------------- | ---
| Branch?       | 2.8
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

---

The release of https://github.com/pdezwart/php-amqp/ 1.7.0alpha1
changed internally the handling of AMQP* object. So now when dumping
using var_dump(), many information are available. So many information
are displayed twice. This commit fixes this issue and keeps displaying basic
information for older versions of the lib.

Reference:

* https://pecl.php.net/package-info.php?package=amqp&version=1.7.0alpha1
* pdezwart/php-amqp@314afbc (and next commits)

---

![screenshot4](https://cloud.githubusercontent.com/assets/408368/22700884/53ad5f68-ed5c-11e6-986b-bfdf91640060.png)

![screenshot5](https://cloud.githubusercontent.com/assets/408368/22700890/580353c4-ed5c-11e6-8fc8-c101115b7001.png)

Commits
-------

c553352 [VarDumper] Improve dump of AMQP* Object
f6744d6
@nicolas-grekas nicolas-grekas deleted the lyrixx:caster-amqp branch Feb 11, 2017
@fabpot fabpot referenced this pull request Feb 17, 2017
Merged

Release v3.2.4 #21640

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