Add SnippetAreaController #87

luca-rath · 2021-04-15T10:38:34Z

Added a SnippetAreaController and refactored the SingleSnippetSelectionResolver

fixes #42

luca-rath · 2021-04-15T10:39:41Z

@wachterjohannes @alexander-schranz @nnatter Are you okay with introducing the SnippetResolver service? If so, I will refactor the tests

wachterjohannes · 2021-04-15T11:27:13Z

@luca-rath @alexander-schranz would go with SnippetSerializer (as seen in https://github.com/sulu/SuluHeadlessBundle/blob/0.x/Content/Serializer/ContactSerializer.php)

luca-rath · 2021-04-15T11:29:18Z

First I thought the same, but this is not really a serializer, because it loads a snippet by uuid

wachterjohannes · 2021-04-15T12:27:28Z

@luca-rath hmm OK seems wrong because we used resolver so much in the bundle 🙈

alexander-schranz

Instead of DefaultSnippetManager::load it should use int he content types DefaultSnippetManager::loadIdentifier so we do not load the snippet twice.

alexander-schranz · 2021-04-15T12:33:18Z

Controller/SnippetAreaController.php

+
+        $loadExcerpt = (bool) $this->getRequestParameter($request, 'loadExcerpt', true, false);
+
+        $snippet = $this->defaultSnippetManager->load($webspaceKey, $area, $locale);


Should be loadIdentifier when the resolver only works with uuid

i guess it would be nice to use this method in the SnippetSelectionResolver and SingleSnippetSelectionResolver too

alexander-schranz · 2021-04-15T12:41:20Z

Content/SnippetResolver.php

+     */
+    public function resolve(
+        string $uuid,
+        string $webspaceKey,


As I remember the webspace key is actually not used in the contentMapper and so we don't need it here.

alexander-schranz · 2021-04-15T12:41:35Z

Content/SnippetResolver.php

+        bool $loadExcerpt = false
+    ): array {
+        /** @var SnippetBridge $snippet */
+        $snippet = $this->contentMapper->load($uuid, $webspaceKey, $locale);


null can be given as webspaceKey here

alexander-schranz · 2021-04-15T12:41:39Z

Content/SnippetResolver.php

+
+        if (!$snippet->getHasTranslation() && null !== $shadowLocale) {
+            /** @var SnippetBridge $snippet */
+            $snippet = $this->contentMapper->load($uuid, $webspaceKey, $shadowLocale);


null can be given as webspaceKey here

alexander-schranz · 2021-04-15T12:42:12Z

Controller/SnippetAreaController.php

+
+        $resolvedSnippet = $this->snippetResolver->resolve(
+            $snippet->getUuid(),
+            $webspaceKey,


can be removed as no webspaceKey is needed to load snippets

alexander-schranz · 2021-04-15T12:42:48Z

Content/SnippetResolverInterface.php

+     */
+    public function resolve(
+        string $uuid,
+        string $webspaceKey,


Snippets has no webspacekey so and contentmapper does since long time not longer require it to load something

alexander-schranz · 2021-04-15T12:44:52Z

I would stay with SnippetResolver

niklasnatter · 2021-04-15T13:25:14Z

Content/SnippetResolver.php

+     * @return mixed[]
+     */
+    public function resolve(
+        string $uuid,


i think the other resolvers do not load the data by themselves but get the loaded data passed to the resolve function, eg the StructureResolver receives a StructureBridge $structure here

i guess we should pass a SnippetBridge $snippet here for consistency because of this - what do you think? @sulu/core-team

Yes you're right, but then the functionality regarding loading the snippet in the shadow locale, if the original locale is not available, will be duplicated in the SingleSnippetSelectionResolver and the SnippetSelectionResolver ... But on the other hand, then this service would really just be a serializer

i think it would be okay for me to duplicate the fallback to the shadow locale in the SingleSnippetSelectionResolver and SnippetSelectionResolver. i guess this is only required when a snippet is resolved as part of a page/article, right?

implementing this as SnippetSerializer in the Serializer namespace would feel better for me too. but dont have a too strong opinion 🙂

Then we don't even need a service for that, because we can just use the StructureResolver directly

thats right, sorry 🙈 can you think about a case where we need a shadow locale outside of the SingleSnippetSelectionResolver/SnippetSelectionResolver? @sulu/core-team

if no, i would prefer to duplicate the shadow locale handling in the SingleSnippetSelectionResolver/SnippetSelectionResolver and use the StructureResolver directly in all other cases. that way we dont provide a resolver service that is inconsistent with the other resolvers.

Okay seem slike there is no shadow handling needed in the controller so it sounds good for me if the controller then just does.

$snippet = $defaultSnippetManager->load($defaultSnippet); $structureResolver->resolve($snippet, ...);

And the call just SingleSnippetSelection calls like SingleMediaSelection the SnippetSelection so we have the shadow handling only at this case.

niklasnatter · 2021-04-15T13:29:25Z

Content/ContentTypeResolver/SingleSnippetSelectionResolver.php

@@ -81,18 +72,13 @@ public function resolve($data, PropertyInterface $property, string $locale, arra
            return new ContentView(null, ['id' => null]);


would be nice to use the DefaultSnippetManager::loadIdentifier() method in this class too

niklasnatter · 2021-04-15T13:29:38Z

Content/ContentTypeResolver/SnippetSelectionResolver.php

@@ -79,18 +70,13 @@ public function resolve($data, PropertyInterface $property, string $locale, arra

        $snippets = [];


would be nice to use the DefaultSnippetManager::loadIdentifier() method in this class too

niklasnatter · 2021-04-15T13:48:05Z

Resources/config/routing_website.yml

@@ -7,3 +7,8 @@ sulu_headless.api.search:
    path: /api/search
    defaults:
        _controller: sulu_headless.controller.search:getAction
+
+sulu_headless.api.snippet_area:
+    path: /api/snippet/{area}


not sure if we should use /api/snippet-areas/{area} here. maybe we want to provide an api for loading a snippet by its id in the future? 🤔 @sulu/core-team

Yes, sounds good 👍

@nnatter i would say that would be another API /api/snippets/{id} or /api/snippets/{key} would keep the path /api/snippet-areas/{area}

@wachterjohannes currently it's /api/snippet/{area} and @nnatter suggested using /api/snippet-areas/{area} 🙂

then @nnatter and i want the same 😅

alexander-schranz · 2021-04-16T08:51:04Z

Tests/Application/Testing/HeadlessBundleKernelBrowser.php

+ *
+ * @internal
+ */
+class HeadlessBundleKernelBrowser extends KernelBrowser


This should not be used. The headless bundle works differently to other API's it need to ignore the Accept header and always return the .json also when the browser does not send the Accept header with introducing this KernelBrowser we are not longer testing the HeadlessBundle correctly. This KernelBrowser should be removed and jsonRequest should not be used to test the HeadlessBundle endpoints. Detection on the format is just done by the .json at the end or that the api like navigaiton and snippet just always return a json.

But exceptions are returned as symfony exception page and not as json, if I use $client->request() ... and an url like /api/snippet-areas/default.json won't work, because then symfony assumes default.json is the {area} instead of default

So how can i test the exceptions then?

The exception should still be json. Seems then the _format is not correctly set for this route try add something like:

defaults: _format: json

Check if in the controller $request->getRequestFormat() returns you json. For navigation and snippet its okay to have .json optional e.g.: /api/snippet-areas/{area}.{_format} but the $request->getRequestFormat() should return json.

Did add the same also to the navigation route

niklasnatter · 2021-04-16T08:57:02Z

Content/ContentTypeResolver/SingleSnippetSelectionResolver.php

+        $snippetId = $data ?: null;
+
+        $contentView = $this->snippetSelectionResolver->resolve(
+            $snippetId ? [$snippetId] : null,
+            $property,
+            $locale,
+            $attributes
+        );
+        $content = $contentView->getContent();
+        $view = $contentView->getView();
+
+        return new ContentView(
+            $content[0] ?? null,
+            ['id' => $view['ids'][0] ?? null]
+        );


is there a reason for implementing this different (eg. no early return) to the SingleMediaSelectionResolver and SinglePageSelectionResolver?
guess it will work fine, just curious 🙂

Yes, because if using an early return, if data is null, the default snippet won't be taken into account, because that handling is only in the SnippetSelectionResolver now

makes sense - thanks!

niklasnatter · 2021-04-16T09:10:42Z

Content/ContentTypeResolver/SnippetSelectionResolver.php

-        }
-
-        return $snippet->getUuid();
+        return new ContentView($snippets, ['ids' => $snippetIds ?: []]);


not sure if we returned $data (does not contain default snippet id) instead of $snippetIds (does contain default snippet id) here intentionally. cant think how someone would use this, so its okay for me both ways 🙂

I just thought, it would be confusing, if an actual snippet is returned (the default snippet) but the view data says the id is null

Add SnippetAreaController

1be5047

luca-rath marked this pull request as draft April 15, 2021 10:38

alexander-schranz requested changes Apr 15, 2021

View reviewed changes

alexander-schranz reviewed Apr 15, 2021

View reviewed changes

alexander-schranz requested changes Apr 15, 2021

View reviewed changes

niklasnatter reviewed Apr 15, 2021

View reviewed changes

alexander-schranz added the Feature New functionality not yet included in the Bundle label Apr 15, 2021

luca-rath added 4 commits April 15, 2021 17:50

Implement review and add test cases

0379812

Revert UPGRADE.md

f31bf12

Refactor SingleSnippetSelectionResolver

09b7c7b

Fix HeadlessBundleKernelBrowser

7dd0a84

luca-rath marked this pull request as ready for review April 15, 2021 16:14

Fix HeadlessBundleKernelBrowser

40a38cd

luca-rath requested a review from alexander-schranz April 15, 2021 16:46

luca-rath added 2 commits April 15, 2021 18:56

Fix tags

31fa401

Add test for missing snippet in specific locale

03e5daf

alexander-schranz reviewed Apr 16, 2021

View reviewed changes

niklasnatter reviewed Apr 16, 2021

View reviewed changes

Remove HeadlessBundleKernelBrowser

8e5e9c5

alexander-schranz approved these changes Apr 16, 2021

View reviewed changes

wachterjohannes merged commit 83562fa into sulu:0.x Apr 16, 2021

luca-rath deleted the feature/snippet-area-controller branch April 16, 2021 11:04


		$loadExcerpt = (bool) $this->getRequestParameter($request, 'loadExcerpt', true, false);

		$snippet = $this->defaultSnippetManager->load($webspaceKey, $area, $locale);

		@@ -81,18 +72,13 @@ public function resolve($data, PropertyInterface $property, string $locale, arra
		return new ContentView(null, ['id' => null]);

		@@ -79,18 +70,13 @@ public function resolve($data, PropertyInterface $property, string $locale, arra

		$snippets = [];

Add SnippetAreaController #87

Add SnippetAreaController #87

Conversation

luca-rath commented Apr 15, 2021 • edited

luca-rath commented Apr 15, 2021

wachterjohannes commented Apr 15, 2021

luca-rath commented Apr 15, 2021

wachterjohannes commented Apr 15, 2021

alexander-schranz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-schranz commented Apr 15, 2021

Choose a reason for hiding this comment

niklasnatter Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasnatter Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasnatter Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexander-schranz Apr 16, 2021 • edited

Choose a reason for hiding this comment

luca-rath Apr 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasnatter Apr 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca-rath commented Apr 15, 2021 •

edited

niklasnatter Apr 15, 2021 •

edited

niklasnatter Apr 15, 2021 •

edited

niklasnatter Apr 15, 2021 •

edited

alexander-schranz Apr 16, 2021 •

edited

luca-rath Apr 16, 2021 •

edited

niklasnatter Apr 16, 2021 •

edited