Skip to content

Conversation

@iulyanp
Copy link
Contributor

@iulyanp iulyanp commented May 4, 2017

Hello,

I changed the visibility of the $children property and use the getLastName method in order for the example to work correctly.

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

👍 these fixes look good to me. Thanks!

public function setLastName($name)
public function getLastName()
{
$this->lastName = $name;
Copy link
Contributor

Choose a reason for hiding this comment

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

You should change this to return $this->lastName; then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks

public $children = array();

public function setLastName($name)
public function getLastName()
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. This section is about modifying objects not reading from them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this looks better.


public function getChildren() {
return $this->children;
}
Copy link
Member

Choose a reason for hiding this comment

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

I would move both methods between setLastName() and __set(). And to match the code style of the Symfony code the opening curly brace should be on its own line. However, we can easily make this change while merging your PR.

Copy link
Member

@xabbuh xabbuh 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

@Simperfit
Copy link
Contributor

This could be merged 👍 @javiereguiluz

@javiereguiluz
Copy link
Member

@iulyanp thanks a lot for contributing these fixes and congrats on your first Symfony Docs contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

6 participants