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.

Sign up for GitHub

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

Jump to bottom

Add AdvancedNamingStrategyInterface #859

Merged
merged 5 commits into from
Feb 4, 2018
Merged

Add AdvancedNamingStrategyInterface #859

merged 5 commits into from
Feb 4, 2018

Conversation

raskyer
Copy link
Contributor

@raskyer raskyer commented Jan 18, 2018

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #857
License Apache-2.0

goetas
goetas requested changes Jan 18, 2018
View reviewed changes
Copy link
Collaborator

@goetas goetas left a comment

Choose a reason for hiding this comment

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

Here is my review. The main change is that you can change the signature of the constructor

src/JMS/Serializer/AbstractVisitor.php Outdated
protected $advancedNamingStrategy;

public function __construct(
PropertyNamingStrategyInterface $namingStrategy,
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of adding a new parameter, you can remove the signature from the current

src/JMS/Serializer/AbstractVisitor.php Outdated
/**
* @return bool
*/
protected function hasAdvancedNamingStrategy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

you do not need this

src/JMS/Serializer/GenericDeserializationVisitor.php Outdated
@@ -161,6 +161,9 @@ public function startVisitingObject(ClassMetadata $metadata, $object, array $typ
public function visitProperty(PropertyMetadata $metadata, $data, Context $context)
{
$name = $this->namingStrategy->translateName($metadata);
if ($this->hasAdvancedNamingStrategy()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

better do do if ( instanceof ...) then "new strategy" else "old strategy" endif instead of computing each time the name twice

src/JMS/Serializer/GenericSerializationVisitor.php Outdated
@@ -158,6 +158,9 @@ public function visitProperty(PropertyMetadata $metadata, $data, Context $contex
}

$k = $this->namingStrategy->translateName($metadata);
if ($this->hasAdvancedNamingStrategy()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as said on other comment

src/JMS/Serializer/JsonSerializationVisitor.php Outdated
@@ -165,6 +165,9 @@ public function visitProperty(PropertyMetadata $metadata, $data, Context $contex
}

$k = $this->namingStrategy->translateName($metadata);
if ($this->hasAdvancedNamingStrategy()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as said on other comment

src/JMS/Serializer/YamlSerializationVisitor.php Outdated
{
parent::__construct($namingStrategy, $accessorStrategy);
public function __construct(
PropertyNamingStrategyInterface $namingStrategy,
Copy link
Collaborator

Choose a reason for hiding this comment

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

as said on other comment

src/JMS/Serializer/YamlSerializationVisitor.php Outdated
@@ -166,6 +170,9 @@ public function visitProperty(PropertyMetadata $metadata, $data, Context $contex
}

$name = $this->namingStrategy->translateName($metadata);
if ($this->hasAdvancedNamingStrategy()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as said on other comment

src/JMS/Serializer/SerializerBuilder.php Outdated
@@ -205,6 +207,13 @@ public function setPropertyNamingStrategy(PropertyNamingStrategyInterface $prope
return $this;
}

public function setAdvancedNamingStrategy(AdvancedNamingStrategyInterface $advancedNamingStrategy)
{
$this->advancedNamingStrategy = $advancedNamingStrategy;
Copy link
Collaborator

Choose a reason for hiding this comment

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

here you might overwrite $this->propertyNamingStrategy

src/JMS/Serializer/SerializerBuilder.php Outdated
'xml' => new XmlSerializationVisitor($this->propertyNamingStrategy, $this->getAccessorStrategy()),
'yml' => new YamlSerializationVisitor($this->propertyNamingStrategy, $this->getAccessorStrategy()),
'json' => new JsonSerializationVisitor($this->propertyNamingStrategy, $this->getAccessorStrategy()),
'xml' => new XmlSerializationVisitor(
Copy link
Collaborator

Choose a reason for hiding this comment

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

so no need to change this code

src/JMS/Serializer/SerializerBuilder.php Outdated
@@ -241,8 +262,16 @@ public function addDefaultDeserializationVisitors()

$this->visitorsAdded = true;
$this->deserializationVisitors->setAll(array(
'xml' => new XmlDeserializationVisitor($this->propertyNamingStrategy),
'json' => new JsonDeserializationVisitor($this->propertyNamingStrategy),
'xml' => new XmlDeserializationVisitor(
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this

@raskyer
Copy link
Contributor Author

raskyer commented Jan 18, 2018

@goetas Fix for your first request

goetas
goetas requested changes Jan 18, 2018
View reviewed changes
Copy link
Collaborator

@goetas goetas left a comment

Choose a reason for hiding this comment

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

Looks much better now.
Please add also some tests.

The current naming strategies might implement both

src/JMS/Serializer/Naming/AdvancedNamingStrategyInterface.php Outdated
*
* @return string
*/
public function translateName(PropertyMetadata $property, Context $context);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, my bad, please use a different name (as example getPropertyName, so people can implement both interfaces (for compatibility)

@raskyer
Copy link
Contributor Author

raskyer commented Jan 18, 2018

@goetas Should be ok right now

@goetas goetas self-requested a review January 18, 2018 15:09
goetas
goetas approved these changes Jan 18, 2018
View reviewed changes
Copy link
Collaborator

@goetas goetas left a comment

Choose a reason for hiding this comment

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

looks good to me for now. will check when can be merged this

@goetas goetas self-assigned this Jan 18, 2018
@goetas goetas added this to the 1.11 milestone Feb 3, 2018
@goetas goetas mentioned this pull request Feb 4, 2018
Closed
@goetas goetas merged commit 6faeb47 into schmittjoh:master Feb 4, 2018
@goetas
Copy link
Collaborator

goetas commented Feb 4, 2018

Thanks!

@goetas goetas mentioned this pull request Apr 16, 2018
Closed
@goetas goetas mentioned this pull request May 26, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goetas goetas goetas approved these changes

Assignees

@goetas goetas

Labels
enhancement
Projects
None yet
Milestone
1.11
Development

Successfully merging this pull request may close these issues.
2 participants
@raskyer @goetas