Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#16 | devel-require-var-dumper-2987782-16.patch | 951 bytes | willzyx |
| |||
#11 | devel-require-var-dumper-2987782-10.patch | 2.22 KB | lussoluca |
| |||
#9 | devel-require-var-dumper-2987782-9.patch | 448 bytes | lussoluca |
#6 | devel-require-var-dumper-2987782-6.patch | 2.22 KB | lussoluca |
#4 | devel-require-var-dumper-2987782-4.patch | 1.02 KB | lussoluca |
Comments
Comment #2
lussolucaComment #4
lussolucaComment #6
lussolucaComment #7
moshe weitzman CreditAttribution: moshe weitzman commentedI 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?
Comment #8
lussolucaIn a Symfony app it is required by the symfony/framework-bundle package that we doesn't use in Drupal
Drupal 8.4 was using 3.2. Maybe we can use the same dependency constraint as drush: "symfony/var-dumper": "~2.7|^3" ?
Comment #9
lussolucaComment #11
lussolucaComment #13
lussolucaCommitted and pushed to 8.x-1.x
Comment #15
willzyx CreditAttribution: willzyx commented@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.
Comment #16
willzyx CreditAttribution: willzyx commentedManually 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
Comment #17
willzyx CreditAttribution: willzyx commentedIf no one have objections i'm going to commit this i the next few days
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commented@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.
Comment #19
lussolucaOf 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).
Comment #20
moshe weitzman CreditAttribution: moshe weitzman commentedwe now require var dumper