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

Be more intelligent in determining property writable/readable link. #1974

Conversation

Projects
None yet
4 participants
@bendavies
Copy link
Contributor

bendavies commented May 24, 2018

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? N/A yet
Fixed tickets
License MIT
Doc PR

I've come across an issue where I cannot prevent a property being a readable link, given sane looking serialization Group configuration.

Consider the following Resources:

/**
 * @ApiResource(
 *     attributes={
 *         "normalization_context": {"groups": {"foo-read", "bar-read"}},
 *     }
 * )
 */
class Foo
{
    /**
     * @Groups("foo-read")
     *
     * @var Bar
     */
    public $id;

    /**
     * @Groups("foo-read")
     *
     * @var Bar
     */
    public $bar;
}

/**
 * @ApiResource()
 */
class Bar
{
    /**
     * @Groups("foo-read")
     *
     * @var Bar
     */
    public $id;

    /**
     * @Groups("bar-read")
     *
     * @var Foo
     */
    public $foo;
}

Given:

$foo = new Foo();
$foo->id = 1;

$bar = new Bar();
$bar->id = 1;

$foo->bar = $bar;
$bar->foo = $foo;

Consider normalizing $foo.

What i want (and would expect) is
Foo::bar to be readable: true readableLink: true
Foo::bar:foo to be readable: true readableLink: false

but what currently is returned is
Foo::bar to be readable: true readableLink: true
Foo::bar:foo to be readable: true readableLink: true

I need Foo::bar:foo readableLink: false so the normalization does not recurse back to Foo: Foo->Bar->Foo, it should stop at Foo->Bar.

SerializerPropertyMetadataFactory::transformLinkStatus does not consider the groups on the property it is evaluating the link status of. I think that would be expected, and this change would give my desired outcome.

@bendavies bendavies changed the title Be more intelligent and Be more intelligent in determining property writable/readable link. May 24, 2018

@dunglas

This comment has been minimized.

Copy link
Member

dunglas commented May 24, 2018

I gave only a quick look but looks legit. ping @api-platform/core-team

@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented May 24, 2018

This looks good to me indeed. I'd see this as a bug fix (target 2.2) though.

@bendavies

This comment has been minimized.

Copy link
Contributor Author

bendavies commented May 24, 2018

Ah I did mean to target 2.2. 👍

@bendavies

This comment has been minimized.

Copy link
Contributor Author

bendavies commented May 24, 2018

I'll work on the tests.

@bendavies bendavies changed the base branch from master to 2.2 May 24, 2018

@antograssiot

This comment has been minimized.

Copy link
Member

antograssiot commented Apr 6, 2019

working on finishing this one

@bendavies

This comment has been minimized.

Copy link
Contributor Author

bendavies commented Apr 6, 2019

Wow forgot about this one - thanks!

@antograssiot antograssiot force-pushed the bendavies:serializer-property-metadata-factory-writable-link-fix branch from 98e237c to e8b79dc Apr 6, 2019

@antograssiot antograssiot changed the base branch from 2.2 to master Apr 6, 2019

@antograssiot

This comment has been minimized.

Copy link
Member

antograssiot commented Apr 6, 2019

targeting master as discussed with @soyuka.
Even if we think it is a bug fix, it might be totally misleading to people relying up to now on this "faulty" behavior

@antograssiot antograssiot force-pushed the bendavies:serializer-property-metadata-factory-writable-link-fix branch from e8b79dc to d94c1a6 Apr 6, 2019

@soyuka soyuka merged commit e03b925 into api-platform:master Apr 6, 2019

11 checks passed

Scrutinizer Analysis: 1 updated code elements – Tests: passed
Details
SymfonyInsight: dunglas / API Platform Core Code quality OK.
Details
ci/circleci: behat-coverage Your tests passed on CircleCI!
Details
ci/circleci: merge-and-upload-coverage Your tests passed on CircleCI!
Details
ci/circleci: php-cs-fixer Your tests passed on CircleCI!
Details
ci/circleci: phpstan Your tests passed on CircleCI!
Details
ci/circleci: phpunit-coverage Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 95.7%)
Details
codecov/project 95.7% (+<.01%) compared to 5f7eaf3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented Apr 6, 2019

@bendavies

This comment has been minimized.

Copy link
Contributor Author

bendavies commented Apr 6, 2019

Any tests added for this?

@bendavies

This comment has been minimized.

Copy link
Contributor Author

bendavies commented Apr 6, 2019

Commit php-cs-fixer by accident?

@soyuka

This comment has been minimized.

Copy link
Member

soyuka commented Apr 6, 2019

Any tests added for this?

Yes in fact it fixes a bad behavior that is shown by the modified groups (behat functional tests).

@vincentchalamon vincentchalamon referenced this pull request Apr 6, 2019

Merged

Fix typos #2702

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