NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[WIP][Serializer]Use PropertyInfo to extract properties#28775

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

fbourigault wants to merge8 commits intosymfony:masterfromfbourigault:object-normalizer-property-list-extractor

Closed

[WIP][Serializer]Use PropertyInfo to extract properties#28775

fbourigault wants to merge8 commits intosymfony:masterfromfbourigault:object-normalizer-property-list-extractor

Conversation

Copy link

Contributor

fbourigault commentedOct 8, 2018•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	?
License	MIT
Doc PR	symfony/symfony-docs#10530

This is a small refactoring to reduce the complexity of the ObjectNormalizer. It now use aPropertyListExtractorInterface to get the class properties list.

To achieve this, I also added aexclude_static_properties option to theReflectionExtractor::getProperties method.

Starting from this PR,symfony/serializer has a hard dependency onsymfony/property-info.

This was exposed by@dunglas in#19374 (comment).

To do

add a CHANGELOG.md entry.
documentation forexclude_static_properties.
check for performance regression.

fbourigault requested a review fromdunglas as acode owner

October 8, 2018 20:55

carsonbot added Status: Needs Review Serializer Feature labels

Oct 8, 2018

dunglas requested changes

Oct 8, 2018

View reviewed changes

Copy link

Member

dunglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Awesome, it will allow to dramatically simplify the component, then to improve its performance.

src/Symfony/Component/PropertyInfo/Tests/Extractor/ReflectionExtractorTest.php OutdatedShow resolvedHide resolved

src/Symfony/Component/PropertyInfo/Extractor/ReflectionExtractor.php

		$properties =array();
		foreach ($reflectionPropertiesas$reflectionProperty) {
		if ($reflectionProperty->isPublic()) {
		if ($reflectionProperty->isPublic() && (!($context[self::EXCLUDE_STATIC_PROPERTIES] ??false) \|\| !$reflectionProperty->isStatic())) {

Copy link

Member

dunglasOct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It almost looks like a bug to me that we used to includestatic properties. I would like to change the default behavior and add a flag to include static props instead, but it's maybe to late (BC break). WDYT @symfony/deciders?