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

Enhanced location handling (EZP-32182) #90

Merged
merged 9 commits into from
Dec 10, 2020

Conversation

bdunogier
Copy link
Member

@bdunogier bdunogier commented Nov 25, 2020

A major refactoring of the GraphQL API that bumps it to v3.0.

*Content as well as the whole DomainContent terminology is replaced with a concept of Item, where an Item is the combination of a content and a location. Example: "ArticleContent" becomes "ArticleItem".

The goal is to add a concept of "current location" so that from any retrieved item, we can get a location (and its URL). It uses a Location Guesser API that given a Content item will guess a Location for it. It allows to keep the API content centric while allowing to work with locations in a smart way.

Features

Illustration of the changes below:

  • there are two public siteaccesses, site1 and site2
  • the query is made from site1.
  • folderBlog 135 has two locations, one in site1, one in site2.
  • folderBlog.posts is a query field that returns the blog posts that are children of the blog's current location.
  • blog.relatedBlogPosts contains relations to both both the site1 and site2 subtrees
    Result:
  • only the posts from the location in site1 are returned
  • their URL is relative to site1 (no /site-1 prefix)
  • the URL for the related blog post that is in site2 is an absolute one, since it is on another siteaccess
{
  content {
    folderBlog(contentId: 135) {
      _name
      _url
      posts {
        edges {
          node {
            _url
            relatedBlogPosts {
              _url
            }
          }
        }
      }
    }
  }
}
{
  "data": {
    "content": {
      "folderBlog": {
        "_name": "Blog",
        "_url": "/blog",
        "posts": {
          "edges": [
            {
              "node": {
                "_url": "/blog/blog-post-site-2",
                "relatedBlogPosts": [
                  {
                    "_url": "https://site1.ezcommerce.local/blog/blog-post-site-1"
                  },
                  {
                    "_url": "/blog/friday-tech-beer"
                  }
                ]
              }
            },
            {
              "node": {
                "_url": "/blog/friday-tech-beer",
                "relatedBlogPosts": []
              }
            }
          ]
        }
      }
    }
  }
}

Tree root support

Tree root settings are now taken into account automatically:

  • only items that have a valid location in the current siteaccess are returned (e.g. within the subtree location id or within excluded paths)
  • url aliases are stripped from the tree root prefix (e.g. "/site-1/item" => "/item")
  • items can be searched by url alias without the tree root prefix ("/item" to search for "/site-1/item" when on siteaccess site1)

Item objects

When working with content, and Item value object is used. It references either a location or a content, and guesses the other:

  • If it is built with a Location, the content is retrieved from the Location.
  • If it is built with a Content, the Location is guessed using the Location Guesser API

A generic resource, item, with the same arguments than article or folder, is also available. It returns the item object, and its type can be tested in the query. It is meant to limit the usage of the _repository { location resource.

Location guesser API

Given a Content item, the LocationGuesser will return the "best" location for it. It uses a set of filters on the content item's locations:

  • only locations valid for the current siteaccess
  • if several, the main one.

More of those filters could be added:

  • within a given subtree (would require that a "Context" is provided)
  • lowest priority
  • etc.

Siteaccess guesser API

A specific ItemFactory, $relatedItemFactory, will accept locations from other siteaccesses. When an URL for such an item is requested, it will be generated for the Item's siteaccess, using an absolute URL if necessary.

Other minor changes

  • The id argument for loading an item is replaced with contentId. locationRemoteId and urlAlias have also been added as arguments

TODO

  • Fix CS
  • Apply the same change to the mutation terminology (createArticleContent -> createArticleItem etc)
  • Apply to type handling as well ?
  • Change Item { _locations } so that it returns the locations that are valid for the item
  • Add a shortcut to get the url alias for a location. Example: Item { _locations { _url } }
  • Add an item resource that returns an item independently of its type (use-case: search for any item with an URL alias)
  • Decide what to do with Related Items Locations: do we also restrict the results to the current siteaccess ? Or do we want some way to allow external locations ? Is this a per-field thing ? Or should we just follow permissions (e.g. if you're allowed to view it, you can get a link to it) ?

@bdunogier bdunogier force-pushed the ezp32182-enhanced_location_handling branch 2 times, most recently from 2243a88 to 6342d46 Compare November 25, 2020 14:22
@bdunogier bdunogier mentioned this pull request Nov 25, 2020
4 tasks
@bdunogier bdunogier force-pushed the ezp32182-enhanced_location_handling branch from 6342d46 to 5f919be Compare November 25, 2020 15:25
@bdunogier bdunogier requested a review from webhdx November 25, 2020 18:37
@bdunogier bdunogier marked this pull request as draft November 25, 2020 18:37
src/GraphQL/Resolver/ItemResolver.php Outdated Show resolved Hide resolved
src/GraphQL/Resolver/ItemResolver.php Outdated Show resolved Hide resolved
src/GraphQL/ItemFactory.php Outdated Show resolved Hide resolved
src/GraphQL/InputMapper/SearchQueryMapper.php Outdated Show resolved Hide resolved
Comment on lines 157 to 162
return array_map(
function ($contentId) use ($contentItems) {
return $contentItems[array_search($contentId, array_column($contentItems, 'id'))];
},
$destinationContentIds
);
Copy link
Member

Choose a reason for hiding this comment

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

At the moment whole expression could be reduced to return $contentItems Did you mean

return array_map(
    function (Content $content): Item {
        return $this->itemFactory->fromContent($content);
    },
    $contentItems
);     

?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I recall correctly (I wrote that a while ago) the goal was to return the items in the order they are defined in the relation field. And if I recall correctly (again), the search result doesn't respect the order the id are passed in.

Copy link
Member

Choose a reason for hiding this comment

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

the search result doesn't respect the order the id are passed in.

True, especially in multi node setup.

src/GraphQL/Resolver/LocationGuesser/LocationList.php Outdated Show resolved Hide resolved
Comment on lines +119 to +117
private function containsRootPath(array $path, array $rootPath): bool
{
return array_slice($path, 0, count($rootPath)) === $rootPath;
}
Copy link
Member

Choose a reason for hiding this comment

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

We should have Path VO into PAPI to avoid re-implementing path operation at-hoc...

Copy link
Member Author

Choose a reason for hiding this comment

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

Fine by me.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could even go further, and have an official API for getting the URL alias for a location for a given siteaccess.

src/GraphQL/Resolver/UrlAliasResolver.php Outdated Show resolved Hide resolved
Comment on lines 30 to 43
if ($location === null && $content === null) {
// @todo find a better exception
throw new \Exception('Constructing an item requires one of location or content');
}
Copy link
Member

Choose a reason for hiding this comment

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

... or mark constructor as private/protected and force class instantiation using named constructors:

public static fromContent(LocationGuesser $locationGuesser, Content $content): self;
public static fromLocation(LocationGuesser $locationGuesser, Content $location): self;

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, why not. Do we agree that I still keep the ItemFactory, but it uses the static methods instead of the constructor ?

@bdunogier bdunogier force-pushed the ezp32182-enhanced_location_handling branch 5 times, most recently from f349e72 to eebf539 Compare November 26, 2020 15:40
@bdunogier bdunogier marked this pull request as ready for review November 26, 2020 15:51
@bdunogier
Copy link
Member Author

We should be green. There are still todo items above, but they may also be taken care of in a follow-up. I'll work on them anyway, but if we could consider this PR for merging, it would be nice.

@bdunogier bdunogier requested a review from alongosz November 26, 2020 15:52
@bdunogier bdunogier force-pushed the ezp32182-enhanced_location_handling branch 2 times, most recently from 6eb2633 to 0102724 Compare November 27, 2020 16:34
@bdunogier
Copy link
Member Author

To reviewers, I'd like to merge this as soon as possible and iterate further. There are remaining tasks, but it's a large refactoring, and doing it all in one pull-requests will make it difficult.

@bdunogier bdunogier force-pushed the ezp32182-enhanced_location_handling branch 2 times, most recently from 0f5b618 to 5892ade Compare November 30, 2020 10:02
Bertrand Dunogier added 6 commits November 30, 2020 15:13
Two ItemFactory instances are exposed, depending on the need:
- $siteItemFactory: locations and aliases are within the current site root only
- $relatedItemFactory: locations and aliases can be in other public siteaccesses / site roots
@bdunogier bdunogier force-pushed the ezp32182-enhanced_location_handling branch from 53f2e4e to 085a6dd Compare November 30, 2020 14:13
@bdunogier
Copy link
Member Author

Summary of the plan for this:

  • this is a major change, a BC break from GraphQL 2.x
  • the idea is/was to release a 3.0-beta that can be installed on the LTS, and will be stabilized for the 4.0 release

If the plan doesn't work, please let me know. But releasing a BC break of that package in the LTS doesn't sound correct.

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

Successfully merging this pull request may close these issues.

2 participants