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

add Extension System for StructureResolver #100

Open
wants to merge 1 commit into
base: 0.10
Choose a base branch
from

Conversation

j-mahlke
Copy link

In our last projects we often have replaced the StructureResolve just because there is no extension point to extend the data there in a easy way. In non "headless" projects, the approach would be to add a TwigExtensions which makes specific data available in all twig templates. This PR makes it possible to extend structures without overriding the class, and therefore allows different bundles to extend the returned data without conflicts.

@alexander-schranz
Copy link
Member

Hello @jmahlkeXTAIN,

Thank you for the pull request, this looks very interesting.

I would like to understand the usecases you had in your project to understand it better where this feature would be come into play. What you did extend and how you did extend it currently. What did in your cases speak against configure a custom controller in your templates. Also in case of sulu websites in a none headless way a custom controller is normally the choice to add additional data to the template twig file, twig extension is mostly the last thing, equivalent to twig extension in headless bundle would be a custom api endpoint.

@j-mahlke
Copy link
Author

Hello @alexander-schranz ,

Our current Project has a React-Frontend that builds on the base of the json from the headless bundle. As it updates itself every 5 seconds with a GET request on the json, more requests per update for more api endpoints would have a strong impact on performance.

As a data example I've created a separate pull-request !101, that uses the functionality from this PR and adds information on the available localizations, that we use for a language-selector in the frontend.

Currently this data is added by extending the StructureResolver. As we have more Project-specific data to add (eg. custom sulu-instance settings), every extension would be in the same file instead of a separate file in the domain folders where it should be.

@Deltachaos
Copy link

@alexander-schranz any update on this?

@alexander-schranz alexander-schranz added the To Discuss The core team has to decide if this will be implemented label Feb 23, 2022
@alexander-schranz
Copy link
Member

Sorry for the late response. The structure resolver is in my point of view not the correct place to add things to the response. As it is here to resolve the structure which includes page selections, smart content, load page specific fields. So not only page responses. The extension would also be called for that field / content types which could accidently end in performance issues if somebody only wanted to add something to the page response.

Sulu has an extra possibility to set the controller per template to add additional data to it. In a headless setup the controller is in all template the HeadlessWebsiteController. It is common that when you need extra data that you are extending from the default controller in this case the HeadlessWebsiteController and add your additional data to it in your custom controller. In this case by extending the resolveStructure method:

protected function resolveStructure(StructureInterface $structure): array

As an example:

class CustomController extends HeadlessWebsiteController
{
    protected function resolveStructure(StructureInterface $structure): array
    {
        $resolveData = parent::resolveStructure($structure);
        
        // add your data here
        
        return $resolveData;
    }
}

You then can configure your custom controller in the template:

<controller>App\YourNameSpace\CustomController</controller>

And so extend the page response with all data you want. That is also the recommended way when you are not in the headless setup and a template requires additional data, see for the none headless solution this documentation.

Copy link
Member

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

Sorry for the inactivity here. As I still recommend to usage of a custom controller to add data to your response. We got a usage for a extension system to implement breadcrumbs #70. Let us know if you want to adopt your pull request to that usage or if some of us should take over. Thank you already for all your effort yo udid put into contributing this feature.

@@ -95,6 +103,10 @@ public function resolve(
$data['view'][$property->getName()] = $contentView->getView();
}

foreach ($this->structureResolverExtensions as $structureResolverExtension) {
$data[$structureResolverExtension->getKey()] = $structureResolverExtension->resolve($requestedStructure, $locale);
Copy link
Member

Choose a reason for hiding this comment

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

As written in the other comments we need to go here with a array_merge/array_replace as there will be multiple extensions adding a breadcrumb for example.

Suggested change
$data[$structureResolverExtension->getKey()] = $structureResolverExtension->resolve($requestedStructure, $locale);
$data = \array_replace($data, $structureResolverExtension->resolve($requestedStructure, $locale));

Comment on lines +20 to +21
public function getKey(): string;

Copy link
Member

Choose a reason for hiding this comment

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

The getKey can not be used as we will have different provider providing a breadcrumb depending on instance of structure.

Suggested change
public function getKey(): string;

Comment on lines +22 to +25
public function resolve(
StructureInterface $requestedStructure,
string $locale
): mixed;
Copy link
Member

Choose a reason for hiding this comment

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

We would more go here with array as return value and use beneath array_merge so all extension can manipulate different data. This is the case for breadcrumb which can be set by multiple extensions e.g. PageBreadcrumb service will do:

if ($structure is not page structure) {
    return [];
}

return [
    'breadcrumbs' => $this->navigationMapper->getBreadcrumb($structure->getUuid()),
];

and the ARticleBreadcrumbProvider (living in the article bundle):

if ($structure is not article structure) {
    return [];
}

$parent = $structure->getPageTreeParent();

if (!$parent) {
    return [];
}

return [
    'breadcrumbs' => $this->navigationMapper->getBreadcrumb($parentarent()->getUuid()),
];
Suggested change
public function resolve(
StructureInterface $requestedStructure,
string $locale
): mixed;
/**
* @return array<string, mixed>
*/
public function resolve(StructureInterface $requestedStructure, string $locale): array;

Comment on lines +369 to +370
### Extending the StructureResolver
To add more data to the json that is returned for a site, you can extend the StructureResolver by registering an service with the tag `sulu_headless.structure_resolver_extension`.
Copy link
Member

Choose a reason for hiding this comment

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

Some markdown viewers require a empty line after a headline to work correctly.

Suggested change
### Extending the StructureResolver
To add more data to the json that is returned for a site, you can extend the StructureResolver by registering an service with the tag `sulu_headless.structure_resolver_extension`.
### Extending the StructureResolver
To add more data to the json that is returned for a site, you can extend the StructureResolverExtension implementing the `StructureResolverExtensionInterface`. If autowire is not activated for your project you need to tag the service with `sulu_headless.structure_resolver_extension`.

Comment on lines +377 to +391
public function getKey(): string
{
return 'myKey';
}

public function resolve(
StructureInterface $requestedStructure,
string $locale
): mixed {
return [
'foo' => 'bar',
'fruits' => Fruits::getArray($locale),
'webspaceKey' => $requestedStructure->getWebspaceKey()
];
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public function getKey(): string
{
return 'myKey';
}
public function resolve(
StructureInterface $requestedStructure,
string $locale
): mixed {
return [
'foo' => 'bar',
'fruits' => Fruits::getArray($locale),
'webspaceKey' => $requestedStructure->getWebspaceKey()
];
}
public function resolve(StructureInterface $requestedStructure, string $locale): array
{
return [
'myKey' => [
'foo' => 'bar',
'fruits' => Fruits::getArray($locale),
'webspaceKey' => $requestedStructure->getWebspaceKey(),
],
];
}

@Deltachaos
Copy link

@alexander-schranz we are currently focusing on other stuff, so if you want feel free to make the changes you have proposed based on our work here. Glad to see that you found a purpose for this as well :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
To Discuss The core team has to decide if this will be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants