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

Implement subresource metadata and annotation #1206

Merged
merged 1 commit into from
Jul 3, 2017

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented Jun 27, 2017

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

For now the @ApiSubresource only does the same thing as @ApiProperty(subresource=true). It'll be better with metadata and an annotation class if we want to add features to it in the future.

Related to #1188. ping @pierredup

@@ -33,7 +34,7 @@ class Container
private $id;

/**
* @ApiProperty(subresource=true)
* @ApiProperty(subresource=@ApiSubresource())
Copy link
Member Author

Choose a reason for hiding this comment

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

Should it be a PROPERTY annotation? I don't think we need it, and there is no value in adding metadata factories for subresources IMO. Lmk.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, @ApiSubresource() directly on the property is easier to type, to read and to understand.

* @author Antoine Bluchet <soyuka@gmail.com>
*
* @Annotation
* @Target({"ANNOTATION"})
Copy link
Member

Choose a reason for hiding this comment

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

Should be @Target({"METHOD", "PROPERTY"})

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, my bad.

@teohhanhui
Copy link
Contributor

I suggest we take this opportunity to make it:

/**
 * @ApiSubresource(operations=true)
 */
protected $children;

(open to better name)

@soyuka
Copy link
Member Author

soyuka commented Jun 28, 2017

What does operations trigger?

@teohhanhui
Copy link
Contributor

teohhanhui commented Jun 28, 2017

@ApiSubresource is for configuring a subresource, just like how @ApiProperty is for configuring a property. It also implies that the property is a subresource (what does this do?). The actual configuration is therefore whether to expose the operations (only GET, for now).

@teohhanhui
Copy link
Contributor

On second thought, we should add operations when we actually support overriding the operations. 😆

@soyuka
Copy link
Member Author

soyuka commented Jun 28, 2017

Okay but I suggest we expose operations without having a operations flag. It doesn't make sense to declare ApiSubresource if you don't want operations, does it?

@teohhanhui
Copy link
Contributor

Okay, maybe we need to have a clear definition of what we call a subresource (for our purposes):

A "subresource" is a resource exposed under a path with parameters dependent on another resource.

@soyuka soyuka force-pushed the improve-subresources branch 9 times, most recently from 9568252 to 4f03571 Compare June 29, 2017 09:32
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Ok for me when Travis will be green (see comment).


$collectionType = new Type(Type::BUILTIN_TYPE_OBJECT,
false,
ArrayCollection::class,
Copy link
Member

Choose a reason for hiding this comment

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

Missing use statement (it should made Travis happy btw).

Copy link
Member Author

Choose a reason for hiding this comment

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

last minute test \o/ fixed thx.

@soyuka soyuka force-pushed the improve-subresources branch 2 times, most recently from d5bc50b to a955dac Compare June 29, 2017 13:33
*
* @return SubresourceMetadata|null
*/
public function getSubresource(): SubresourceMetadata
Copy link
Member

Choose a reason for hiding this comment

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

You have to remove the return type here.

*
* @author Antoine Bluchet <soyuka@gmail.com>
*/
final class SubresourceMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm -1 for this. It duplicates information already available from the Type via PropertyMetadata.

Copy link
Member Author

Choose a reason for hiding this comment

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

It does and in the end we'll have less function calls to retrieve the same information.

           $isCollection = $propertyMetadata->getType()->isCollection();
           $subresource = $isCollection ? $propertyMetadata->getType()->getCollectionValueType()->getClassName() : $propertyMetadata->getType()->getClassName();

This will also ease the maintenance to have a SubresourceMetadata class if we need to support subresource related cases in the future no?

@dunglas dunglas merged commit eb47c57 into api-platform:master Jul 3, 2017
@dunglas
Copy link
Member

dunglas commented Jul 3, 2017

Thanks @soyuka!

@soyuka soyuka deleted the improve-subresources branch July 4, 2017 07:04
@ghost
Copy link

ghost commented Nov 6, 2017

Hi!!
It's possible to do the paging with @ApiSubResource?

For example I have Objects that have many comments! and i would like recover the comments through the Object and paging the reponse!!

@soyuka
Copy link
Member Author

soyuka commented Nov 7, 2017

Please open a new issue, and if it's a collection it should already be paginated.

@ghost
Copy link

ghost commented Nov 7, 2017

@soyuka It's already implemted! It was that i didnt see it in the subResource url api_doc.
thanks

hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
Implement subresource metadata and annotation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants