Skip to content
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

[DataCollector] Don't change data type on DataCollector #33114

Closed
wants to merge 1 commit into from

Conversation

@maxhelias
Copy link
Contributor

commented Aug 11, 2019

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

I'm not sure that this is the best solution but it corrects the typing problem of the getData function that does not return an array but a Symfony\Component\VarDumper\Cloner\Data instance

Tell me if this is not good

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

This is not the correct solution indeed. If a type is wrong, it should be fixed in the branch where the mistake is.

@maxhelias maxhelias force-pushed the maxhelias:fix-type-form branch from 76f7286 to 792ff49 Aug 11, 2019

@maxhelias maxhelias changed the base branch from master to 3.4 Aug 11, 2019

@maxhelias maxhelias changed the title [5.0][Form] Fix getData return type [Form] Fix data type on FormDataCollector Aug 11, 2019

@@ -238,7 +238,7 @@ public function serialize()
}
}
return serialize($this->cloneVar($this->data));
return serialize($this->cloneVar($this->data)->getValue(true));
}

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 11, 2019

Member

Can you please confirm that it works on e.g. the demo app? Can you also please push force again to trigger the CI so that tests run?

This comment has been minimized.

Copy link
@maxhelias

maxhelias Aug 11, 2019

Author Contributor

It's not good on the demo, I still have to fix the method WebProfilerExtension::dumpData() which asks for an instance of Data

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 11, 2019

Member

I think this change is wrong actually - we don't want to resolve the Data before serializing it

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Aug 11, 2019

Member

Instead, maybe try calling the method (without any argument) inside getData?
please also check how the other data collector work; is this one the only one that stores a Data object in the data property?

This comment has been minimized.

Copy link
@maxhelias

maxhelias Aug 11, 2019

Author Contributor

It does not work either without any argument.
After some research I noticed on all the other DataCollector the data type never changes but the values ​​that are dump are.
This solution works in demo with the same behavior as before.
What do you think of that ?

@maxhelias maxhelias force-pushed the maxhelias:fix-type-form branch 3 times, most recently from 4ec9cfe to f633b04 Aug 11, 2019

@maxhelias maxhelias changed the title [Form] Fix data type on FormDataCollector [DataCollector] Don't change data type on DataCollector Aug 12, 2019

@@ -39,7 +39,7 @@ class Stub
public $position = 0;
public $attr = [];
private static $defaultProperties = [];
protected static $defaultProperties = [];

This comment has been minimized.

Copy link
@maxhelias

maxhelias Aug 12, 2019

Author Contributor

I changed the visibility because this line catch an Exception : https://github.com/symfony/symfony/pull/33114/files#diff-2e2bdf9c982ba28c381992923d760aceL60

@maxhelias maxhelias force-pushed the maxhelias:fix-type-form branch 2 times, most recently from 94cb781 to 737ac34 Aug 12, 2019

@maxhelias maxhelias force-pushed the maxhelias:fix-type-form branch from 737ac34 to 65aa196 Aug 12, 2019

@maxhelias maxhelias closed this Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.