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

allow more control on GetSetMethodNormalizer #3582

Merged
merged 1 commit into from
Apr 11, 2012
Merged

allow more control on GetSetMethodNormalizer #3582

merged 1 commit into from
Apr 11, 2012

Conversation

tvlooy
Copy link
Contributor

@tvlooy tvlooy commented Mar 12, 2012

Here is an other attempt. You would use this as follows:

$serializer = new \Symfony\Component\Serializer\Serializer(
    array(new \Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer()),
    array('json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder())
);

$callbacks = array('books' => function ($books) { return NULL; });

return new Response(
    $serializer->serialize($paginator->getRows(), 'json', $callbacks),
    200,
    array('Content-Type' => 'application/json')
);

Besides of returning NULL, you could also do things like:

$callbacks = array(
    'books' => function ($books) {
        $ids = array();
        foreach ($books as $book) {
            $ids[] = $book->getId();
        }
        return $ids;
    },
    'author' => function ($author) {
        return $author->getId();
    },
    'creationDate' => function ($creationDate) {
        return $creationDate->format('d/m/Y');
    },
);

The commit is not complete yet. But at this point I am interested in your opinions.

$attributeValue = $this->serializer->normalize($attributeValue, $format);
if (null !== $attributeValue) {
if (array_key_exists($attributeName, $callbacks)) {
$attributeValue = $callbacks[$attributeName]($attributeValue);
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong as it will allow only closures instead of allowing any callable

@lsmith77
Copy link
Contributor

in general i agree that using a callback is a good solution to provide more power without complicating the API or implementation in this case.

please add a test case, this should also help illustrate how this can be used in practice.

@schmittjoh
Copy link
Contributor

Note that your change breaks the API defined by the interface, i.e. someone using this method needs to type-hint the serializer implementation, not the interface.

It also adds a parameter to the public API of the serializer which will only work with one specific normalizer. What if another normalizer needs additional information, should another parameter be added to the serialize method? What about deserialization?

Bottom line is, the serializer component was simply not designed for this kind of thing. I've tried to make it more flexible before creating the bundle, but some things simply cannot be fixed in a sane way.

@tvlooy
Copy link
Contributor Author

tvlooy commented Mar 13, 2012

Would just adding a setCallbacks() to the GetSetMethodNormalizer be a better solution? That doesn't touch the API. I will try to write some tests this evening.

@schmittjoh
Copy link
Contributor

That would definitely be better.

You would then need to retrieve the normalizer instance before calling serialize on the serializer which also leaves a stale taste, but I have no other solution for now.

*
* @param array $callbacks help normalize the result
*/
public function setCallbacks($callbacks)
Copy link
Member

Choose a reason for hiding this comment

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

you should typehint the argument as array

@tvlooy
Copy link
Contributor Author

tvlooy commented Mar 13, 2012

So, this should be it then. Yet an other usage example:

$normalizer = new \Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer();
$normalizer->setCallbacks(
    array(
        'books' => function ($books) {
            $ids = array();
            foreach ($books as $book) {
                $ids[] = $book->getId();
            }
            return $ids;
        },
    )
);
$serializer = new \Symfony\Component\Serializer\Serializer(
    array($normalizer),
    array('json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder())
);

return new Response(
    $serializer->serialize($paginator->getRows(), 'json'),
    200,
    array('Content-Type' => 'application/json')
);

@tvlooy
Copy link
Contributor Author

tvlooy commented Mar 18, 2012

Anything else needed for this to get pulled in?

*/
public function setCallbacks(array $callbacks)
{
$this->callbacks = $callbacks;
Copy link
Contributor

Choose a reason for hiding this comment

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

what about making sure that each callback is_callable ?
that would imply removing the possibility for null value.

@tvlooy
Copy link
Contributor Author

tvlooy commented Mar 19, 2012

Hm, I like to keep it that way because I like the fact that not passing a callable will result in a warning instead of silently skipping it. You don't get that behaviour by treating it as null.

@vicb
Copy link
Contributor

vicb commented Mar 19, 2012

I was unclear: the code should throw an exception when an element is not callable, this is why null will not be supported any more (it is not a callback as the setCallbacks indicate).

They are several way to support the former behavior:

  • the cb can return a defined interface,
  • the cb can throw a defines exc,
  • by adding a setIgnoredAttributes method

Please also squash your commits.

@tvlooy
Copy link
Contributor Author

tvlooy commented Mar 20, 2012

Yes, I like the setIgnoredAttributes solution. I changed it and squashed the commits.

$attributeValue = $method->invoke($object);
if (array_key_exists($attributeName, $this->callbacks)) {
if (!is_callable($this->callbacks[$attributeName])) {
throw new \BadFunctionCallException(sprintf('The given callback for attribute "%s" is not callable.', $attributeName));
Copy link
Contributor

Choose a reason for hiding this comment

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

I would throw an InvalidArgumentExcption in setCallbacks

Copy link
Contributor

Choose a reason for hiding this comment

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

php.net: InvalidArgumentException => "Exception thrown if an argument does not match with the expected value" is more accurate.

if a callback refers to an undefined function

not the case here

if some arguments are missing.

not the case here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yesyes, My bad. fixing it allready ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it now. Thanks for your input

@tvlooy
Copy link
Contributor Author

tvlooy commented Mar 26, 2012

some improvements and squashed the commits

@stof
Copy link
Member

stof commented Apr 3, 2012

@tvlooy Please rebase your branch. It conflicts with master because of the move of tests.

@tvlooy
Copy link
Contributor Author

tvlooy commented Apr 4, 2012

@stof I will do it on saturday, if that is ok with you.

@fabpot
Copy link
Member

fabpot commented Apr 10, 2012

Is it mergeable now? ping @Seldaek, @schmittjoh.

@tvlooy
Copy link
Contributor Author

tvlooy commented Apr 10, 2012

yes, it should be

fabpot added a commit that referenced this pull request Apr 11, 2012
Commits
-------

039ff6f allow more control on GetSetMethodNormalizer by using callback functions and an ignoreAttributes list

Discussion
----------

allow more control on GetSetMethodNormalizer

Here is an other attempt. You would use this as follows:

    $serializer = new \Symfony\Component\Serializer\Serializer(
        array(new \Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer()),
        array('json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder())
    );

    $callbacks = array('books' => function ($books) { return NULL; });

    return new Response(
        $serializer->serialize($paginator->getRows(), 'json', $callbacks),
        200,
        array('Content-Type' => 'application/json')
    );

Besides of returning NULL, you could also do things like:

    $callbacks = array(
        'books' => function ($books) {
            $ids = array();
            foreach ($books as $book) {
                $ids[] = $book->getId();
            }
            return $ids;
        },
        'author' => function ($author) {
            return $author->getId();
        },
        'creationDate' => function ($creationDate) {
            return $creationDate->format('d/m/Y');
        },
    );

The commit is not complete yet. But at this point I am interested in your opinions.

---------------------------------------------------------------------------

by lsmith77 at 2012-03-12T22:53:18Z

in general i agree that using a callback is a good solution to provide more power without complicating the API or implementation in this case.

please add a test case, this should also help illustrate how this can be used in practice.

---------------------------------------------------------------------------

by schmittjoh at 2012-03-13T04:54:33Z

Note that your change breaks the API defined by the interface, i.e. someone using this method needs to type-hint the serializer implementation, not the interface.

It also adds a parameter to the public API of the serializer which will only work with one specific normalizer. What if another normalizer needs additional information, should another parameter be added to the serialize method? What about deserialization?

Bottom line is, the serializer component was simply not designed for this kind of thing. I've tried to make it more flexible before creating the bundle, but some things simply cannot be fixed in a sane way.

---------------------------------------------------------------------------

by tvlooy at 2012-03-13T06:07:45Z

Would just adding a setCallbacks() to the GetSetMethodNormalizer be a better solution? That doesn't touch the API. I will try to write some tests this evening.

---------------------------------------------------------------------------

by schmittjoh at 2012-03-13T16:22:50Z

That would definitely be better.

You would then need to retrieve the normalizer instance before calling ``serialize`` on the serializer which also leaves a stale taste, but I have no other solution for now.

---------------------------------------------------------------------------

by tvlooy at 2012-03-13T21:32:26Z

So, this should be it then. Yet an other usage example:

    $normalizer = new \Symfony\Component\Serializer\Normalizer\GetSetMethodNormalizer();
    $normalizer->setCallbacks(
        array(
            'books' => function ($books) {
                $ids = array();
                foreach ($books as $book) {
                    $ids[] = $book->getId();
                }
                return $ids;
            },
        )
    );
    $serializer = new \Symfony\Component\Serializer\Serializer(
        array($normalizer),
        array('json' => new \Symfony\Component\Serializer\Encoder\JsonEncoder())
    );

    return new Response(
        $serializer->serialize($paginator->getRows(), 'json'),
        200,
        array('Content-Type' => 'application/json')
    );

---------------------------------------------------------------------------

by tvlooy at 2012-03-18T21:16:48Z

Anything else needed for this to get pulled in?

---------------------------------------------------------------------------

by tvlooy at 2012-03-19T18:33:58Z

Hm, I like to keep it that way because I like the fact that not passing a callable will result in a warning instead of silently skipping it. You don't get that behaviour by treating it as null.

---------------------------------------------------------------------------

by vicb at 2012-03-19T23:15:37Z

I was unclear: the code should throw an exception when an element is not callable, this is why `null` will not be supported any more (it is not a callback as the `setCallbacks` indicate).

They are several way to support the former behavior:

* the cb can return a defined interface,
* the cb can throw a defines exc,
* by adding a `setIgnoredAttributes` method

Please also squash your commits.

---------------------------------------------------------------------------

by tvlooy at 2012-03-20T21:02:06Z

Yes, I like the setIgnoredAttributes solution. I changed it and squashed the commits.

---------------------------------------------------------------------------

by tvlooy at 2012-03-26T20:07:36Z

some improvements and squashed the commits

---------------------------------------------------------------------------

by stof at 2012-04-03T22:36:15Z

@tvlooy Please rebase your branch. It conflicts with master because of the move of tests.

---------------------------------------------------------------------------

by tvlooy at 2012-04-04T07:43:47Z

@stof I will do it on saturday, if that is ok with you.

---------------------------------------------------------------------------

by fabpot at 2012-04-10T18:29:30Z

Is it mergeable now? ping @Seldaek, @schmittjoh.

---------------------------------------------------------------------------

by tvlooy at 2012-04-10T18:55:04Z

yes, it should be
@fabpot fabpot merged commit 039ff6f into symfony:master Apr 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants