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

NGSTACK-807 added handler for canonical url #13

Merged
merged 12 commits into from
Dec 7, 2023

Conversation

ljacmi
Copy link

@ljacmi ljacmi commented Dec 7, 2023

@ljacmi ljacmi requested a review from iherak December 7, 2023 13:22
@emodric
Copy link
Member

emodric commented Dec 7, 2023

Is this intented to go here and not netgen/site-bundle ?


declare(strict_types=1);

namespace Netgen\Bundle\OpenGraphBundle\Handler\FieldType;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really a field type handler.

@iherak
Copy link
Member

iherak commented Dec 7, 2023

Is this intented to go here and not netgen/site-bundle ?

I don't mind it either way, but as this does not have any site-bundle specifics, can't see why it wouldn't be here?

- '@Ibexa\Core\Helper\TranslationHelper'
- "@ibexa.api.service.content"
tags:
- { name: netgen_open_graph.meta_tag_handler, alias: app/canonical }
Copy link
Member

Choose a reason for hiding this comment

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

alias should be ngsite/canonical

Copy link
Member

@emodric emodric Dec 7, 2023

Choose a reason for hiding this comment

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

literal/canonical_url actually, in order to follow conventions in bundle.

@emodric
Copy link
Member

emodric commented Dec 7, 2023

can't see why it wouldn't be here?

Because app was used in handler and service name. But yes, it can go here, sure.

use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
use Symfony\Component\Routing\RouterInterface;

class CanonicalUrl extends Handler
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be final

Copy link
Member

Choose a reason for hiding this comment

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

Also does not need to extend Handler, nor implement supports method, since this is not a field type handler. implements HandlerInterface is enough.

@@ -11,3 +11,14 @@ services:

netgen_open_graph.meta_tag_renderer:
class: Netgen\Bundle\OpenGraphBundle\MetaTag\Renderer

app.opengraph.handler.url:
Copy link
Member

Choose a reason for hiding this comment

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

There's handlers.yaml that defines all handlers. This needs to go there. Also needs to be named appropriately netgen_open_graph.handler.literal.canonical_url


app.opengraph.handler.url:
class: Netgen\Bundle\OpenGraphBundle\Handler\FieldType\CanonicalUrl
public: false
Copy link
Member

Choose a reason for hiding this comment

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

No need for public and lazy properties.

lazy: true
arguments:
- "@router"
- '@Ibexa\Core\Helper\TranslationHelper'
Copy link
Member

Choose a reason for hiding this comment

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

Two extra arguments.

@@ -11,3 +11,5 @@ services:

netgen_open_graph.meta_tag_renderer:
class: Netgen\Bundle\OpenGraphBundle\MetaTag\Renderer

Copy link
Member

Choose a reason for hiding this comment

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

Extra whitespace.

@@ -55,3 +55,10 @@ services:
- "@?logger"
tags:
- { name: netgen_open_graph.meta_tag_handler, alias: field_type/ezimage }

netgen_open_graph.handler.literal.canonical_url:
Copy link
Member

@emodric emodric Dec 7, 2023

Choose a reason for hiding this comment

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

Can you move this so it will be grouped together with other literal handlers? :)

public function getMetaTags($tagName, array $params = []): array
{
$value = $this->router->generate(
'ibexa.url.alias',
Copy link
Member

Choose a reason for hiding this comment

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

Final nitpick, we should use Ibexa\Core\MVC\Symfony\Routing\UrlAliasRouter::URL_ALIAS_ROUTE_NAME constant here instead of hardcoded route name.

@emodric emodric merged commit 8a43c60 into master Dec 7, 2023
1 of 5 checks passed
@iherak iherak deleted the NGSTACK-807-add-handler-for-canonical-url branch December 7, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants