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

Location by id with children support #51

Merged
merged 8 commits into from
Sep 13, 2019

Conversation

bdunogier
Copy link
Member

@bdunogier bdunogier commented Jul 5, 2019

JIRA: https://jira.ez.no/browse/EZP-30726
Target version: 1.1 (merges to master for 2.0 release without any issue)

  • Adds Repository.location: Location field that returns a location by id.
  • Adds Location.children that returns a connection with the location's children
  • Adds Location.content: Content
  • Adds ContentInfo.currentVersion: Version
  • Adds Version.creator: User
  • Adds Content._thumbnail: Thumbnail

Also adds page number based pagination support using a new connection field, LocationConnection.pages.

Example:

{
  _repository {
    location(locationId: 2) {
      pathString
      children {
        pages {
          number
          cursor
        }
        edges {
          node {
            content {
              name
              currentVersion {
                creator {
                  name
                }
              }
            }
          }
        }
      }
    }
  }
}

TODO

  • Define a dedicated alias for thumbnails (200x200 pixels)
  • Change Location.id to be an ID, and add Location.locationId with the auto-increment id.

@bdunogier bdunogier force-pushed the feature-ezp30726-location_children branch 3 times, most recently from 5f27782 to 4a4bf3d Compare July 18, 2019 14:16
@bdunogier bdunogier changed the base branch from master to 1.0 July 18, 2019 14:16
@bdunogier bdunogier force-pushed the feature-ezp30726-location_children branch from 4003fd3 to c74168a Compare July 22, 2019 11:38
@DubMan21
Copy link

DubMan21 commented Aug 21, 2019

Hello, it's a good improvement !
But some features would be interesting, being able to query parents and children as on the master branch : "content { TYPE(query: {....}) {} }" .

Especially query by Depth and by ContentType

@bdunogier
Copy link
Member Author

But some features would be interesting, being able to query parents and children as on the master branch : "content { TYPE(query: {....}) {} }" .

Especially query by Depth and by ContentType

Could you please elaborate a bit ? I'm not sure what you mean here. It is already possible to query a type of item.

@DubMan21
Copy link

I currently use "_repository" and "children" to get my content structure tree from eZ Platform with this query :

{
  _repository {
    location(locationId: 2 ) {
      children {
        edges {
          node {
            urlAliases {
              path 
... }

But I get all the Content Type and I would like to filter by one specific content type. We could have a query like this :

{
  _repository {
    location(locationId: 2 ) {
      children**(query: {ContentType: })** {
        edges {
          node {
            urlAliases {
              path 
... }

And for the depth, we could have a parameter for choosing the maximum depth of the query (e.g. fetching the first two levels of children of the content).

@bdunogier
Copy link
Member Author

I see, it makes sense. While it should be fairly easy to implement, I have an alternative solution to that use-case.

Usually, when a content's sub-items of a particular type are retrieved, they have a meaning in terms of content. For instance, the blog_post sub-items of a blog are the posts of that blog. A gallery's image sub-items are the gallery's images. And so on.

I have designed a fieldtype, https://github.com/bdunogier/ezplatform-query-fieldtype, that models exactly this. You can create gallery.images, or blog.posts, and query them, be it with PHP, REST or GraphQL:

{
  content {
    gallery {
      name
      images {
        name
        image {
          variation(identifier: thumbnail) { url alternativeText }
        }
      }
    }
  }
}

The field definition is configured using:

  • a returned type (example: images)
  • a QueryType (example: children of a location of a given type)
  • a mapping of the QueryType's parameters to either static values, or properties / fields from the current content (example: the gallery item's main location id)

I strongly believe that it is a cleaner, more maintainable solution to the use-case you describe. It lets the content architect model the relationships that exist between items, and lets content consumers focus on rendering the content.

I'm available on slack if you need more information.

@bdunogier bdunogier force-pushed the feature-ezp30726-location_children branch 3 times, most recently from 4dba441 to 5b7faec Compare August 27, 2019 10:57
@bdunogier bdunogier force-pushed the feature-ezp30726-location_children branch from 331a7da to 0f3a928 Compare August 27, 2019 15:28
@bdunogier bdunogier requested a review from webhdx August 28, 2019 09:28
@bdunogier bdunogier force-pushed the feature-ezp30726-location_children branch from 0f3a928 to 8b59c5c Compare August 28, 2019 09:33
@bdunogier
Copy link
Member Author

Ping @webhdx for review :-)

src/GraphQL/InputMapper/SearchQuerySortByMapper.php Outdated Show resolved Hide resolved
src/GraphQL/InputMapper/SearchQuerySortByMapper.php Outdated Show resolved Hide resolved
src/GraphQL/Relay/Page.php Outdated Show resolved Hide resolved
src/GraphQL/Relay/PageAwareConnection.php Outdated Show resolved Hide resolved
src/GraphQL/Resolver/ContentThumbnailResolver.php Outdated Show resolved Hide resolved
*/
public function resolveDomainContentItem(Argument $args, $contentTypeIdentifier)
public function resolveDomainContentItem($args, $contentTypeIdentifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did it change in overblog graphql package? I though it would always return an Argument object which implements ArrayAccess if I'm not mistaken.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did change this because in this PR, to resolve a Location's content field, we call the method directly, with an array as the argument. In src/Resources/config/graphql/Location.types.yml: resolve: "@=resolver('DomainContentItem', [{id: value.contentId}, null])".

I agree that it's not the cleanest thing ever, but... 😊

return $this->locationService->loadLocation($id);
} catch (ApiException\InvalidArgumentException $e) {
} catch (ApiException\NotFoundException $e) {
throw new ArgumentsException($e->getMessage(), $e->getCode(), $e);
Copy link
Contributor

Choose a reason for hiding this comment

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

There happens something confusing to me. Why does ArgumentsException extend NotFoundException if it thrown for NotFoundException and for InvalidArgumentException below? To my understanding it should not extend any particular Exception class because it's very general exception. Also do we need specific ArgumentsException class if we have InvalidArgumentException from our API?

Copy link
Member Author

Choose a reason for hiding this comment

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

The general idea is to make that package only throw its own exceptions (it supposedly catches everything else). Why it extends NotFoundException... I don't know. A mistake I guess. I'll change it extend \Exception. Would that work for you ?

Generally speaking, error handling in GraphQL needs to be vastly improved, but I have barely scratched the surface of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

After thinking about it, I'd like to request your approval to leave this aside for now. The reason why it is implemented that way in this loader is that it was implemented that way in the existing ones.

We must find a proper way to implement exception handling in loaders, but I don't think that we must do it in the context of that PR.

@bdunogier bdunogier force-pushed the feature-ezp30726-location_children branch from 8ce9e0e to 664f009 Compare September 4, 2019 17:14
@bdunogier
Copy link
Member Author

Review comments taken care of, except the two I have answered about, @webhdx.
I'm still thinking about the exception topic. Not as easy as it sounds in the end... (ref. the tough error handling topic in the comment above).

@bdunogier
Copy link
Member Author

Also, the PR has been rebased to contain coherent commits.

@bdunogier bdunogier requested a review from webhdx September 5, 2019 12:47
@bdunogier bdunogier force-pushed the feature-ezp30726-location_children branch from 664f009 to 2de3a92 Compare September 9, 2019 12:12
@bdunogier
Copy link
Member Author

bdunogier commented Sep 9, 2019

@ezsystems/qa-team, the baby is yours.

Since the "real" target is ezsystems/ezplatform-admin-ui-modules#200 that targets v3.0, you should test it on the master branch. I have tested it myself, and it merges as is on ezplatform-graphql@master.

Just make sure you're on master, and git merge origin/feature-ezp30726-location_children.

Once approved, we will merge it to 1.0, and then merge 1.0 onto master (2.0-beta).

@bdunogier bdunogier requested a review from a team September 9, 2019 12:16
@micszo micszo self-assigned this Sep 10, 2019
@micszo
Copy link
Member

micszo commented Sep 11, 2019

It would be helpful to document that changes to these items affect the schema (and require regenerating it): content types, types groups, languages, blocks types, images variations.
Also we need to make sure the list is complete. (: \cc @ezsystems/documentation-team

@bdunogier
Copy link
Member Author

bdunogier commented Sep 13, 2019

@micszo I'll rebase the PR to squash the fixup commit (done).

First iterates over fields, and returns the first ezimage or ezimageasset field. If none is found, tries with ezobjectrelation, and returns the first ezimage or ezimageasset field of the related item.

If an image field is found, generates the small variation of it, and returns it as a Thumbnail object with uri, width, height, alternativeText.
@bdunogier bdunogier force-pushed the feature-ezp30726-location_children branch from c41c3d0 to efe498d Compare September 13, 2019 07:15
@micszo micszo removed their assignment Sep 13, 2019
@micszo
Copy link
Member

micszo commented Sep 13, 2019

@lserwatka this one's ready

@lserwatka lserwatka merged commit fbb596f into 1.0 Sep 13, 2019
@lserwatka lserwatka deleted the feature-ezp30726-location_children branch September 13, 2019 10:16
@lserwatka
Copy link
Member

@bdunogier you can merge it up

@lserwatka
Copy link
Member

Merged via 219501b

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

Successfully merging this pull request may close these issues.

5 participants