Problem/Motivation

The latest Symfony RequestDataCollector needs a class from the var-dumper component to correctly serialize data. Without that class the Request data aren't serialized in a Webprofiler profile.

Proposed resolution

Add symfony/var-dumper in the require section of the root composer.json file. At the moment the composer.json already suggest to install var-dumper, with this patch we're going to always require it.

Remaining tasks

Review

User interface changes

none

API changes

none

Data model changes

none

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lussoluca created an issue. See original summary.

lussoluca’s picture

Assigned: Unassigned » moshe weitzman
Status: Active » Needs review
FileSize
447 bytes

Status: Needs review » Needs work

The last submitted patch, 2: devel-require-var-dumper-2987782-2.patch, failed testing. View results

lussoluca’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

Status: Needs review » Needs work

The last submitted patch, 4: devel-require-var-dumper-2987782-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

lussoluca’s picture

moshe weitzman’s picture

I guess Symfony RequestDataCollector doesn't declare this dependency? I'm OK with adding it, but I pause with the 3.4.0 constraint. Some folks may want to run the new devel with an earlier version of Drupal? Was Drupal 8.4 using Symfony 3.4?

lussoluca’s picture

I guess Symfony RequestDataCollector doesn't declare this dependency?

In a Symfony app it is required by the symfony/framework-bundle package that we doesn't use in Drupal

I'm OK with adding it, but I pause with the 3.4.0 constraint. Some folks may want to run the new devel with an earlier version of Drupal? Was Drupal 8.4 using Symfony 3.4?

Drupal 8.4 was using 3.2. Maybe we can use the same dependency constraint as drush: "symfony/var-dumper": "~2.7|^3" ?

lussoluca’s picture

Status: Needs review » Needs work

The last submitted patch, 9: devel-require-var-dumper-2987782-9.patch, failed testing. View results

lussoluca’s picture

Status: Needs work » Needs review
FileSize
2.22 KB

  • lussoluca committed cf7a3c4 on 8.x-1.x
    Issue #2987782 by lussoluca: Add the Symfony var-dumper component as a...
lussoluca’s picture

Status: Needs review » Fixed

Committed and pushed to 8.x-1.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

willzyx’s picture

Status: Closed (fixed) » Needs review

@luca can we have the evidence of the hidden dependency of Symfony RequestDataCollector on var-dumper? I'm looking at Symfony webprofiler code base and seems that it can run without the var-dumper's classes

As I stated in other occasions, I think var-dumper is the best debug lib for this work but I'm really disagree to add this strong dependency and force developer to install it without a good reason considering that devel, since now, is a standalone module without any dependency.

Also if we really add this dependency for any reason IMO it should be added in a new major branch.

willzyx’s picture

Manually tested webprofiler without var-dumper dependency and seems to work correctly

The attached patch move var-dumper lib to "suggest" section in the composer.json

willzyx’s picture

If no one have objections i'm going to commit this i the next few days

moshe weitzman’s picture

@lussoluca - OK with you? I'm fine with whatever you guys decide. I will say that devel users are often not so sophisticated so it can make sense to include "best practices" like our preferred va-dumper library as a dependency, even if its not strictly true.

lussoluca’s picture

Of course Webprofiler in general works without vardumper.

The problem is the Request data collector. In RequestDataCollector::lateCollect() the new profiler uses DataCollector::cloneVar() to create a serializable version of the data to be collected. This in turn uses the VarCloner class of the vardumper component if exists or fallback to the ValueExporter class if not. The output of VarCloner can be later used to reconstruct the PHP data structure used to render the Webprofiler's request panel, with ValueExporter this is not possibile.

If we remove the vardumper dependency we need to remove the request data collector also (but it contains a lot of useful information).

That said we can choose to hide the request data panel only for users that doesn't have the vardumper component available (every user that already has Drush installed as Composer dependency has the vardumper component available).

moshe weitzman’s picture

Status: Needs review » Closed (outdated)

we now require var dumper