-
Notifications
You must be signed in to change notification settings - Fork 6
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
Conversation
Is this intented to go here and not |
|
||
declare(strict_types=1); | ||
|
||
namespace Netgen\Bundle\OpenGraphBundle\Handler\FieldType; |
There was a problem hiding this comment.
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.
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 } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Because |
use Symfony\Component\Routing\Generator\UrlGeneratorInterface; | ||
use Symfony\Component\Routing\RouterInterface; | ||
|
||
class CanonicalUrl extends Handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be final
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two extra arguments.
… removed and class path fixed
…f extending Handler, supports method removed
@@ -11,3 +11,5 @@ services: | |||
|
|||
netgen_open_graph.meta_tag_renderer: | |||
class: Netgen\Bundle\OpenGraphBundle\MetaTag\Renderer | |||
|
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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.
task id: https://netgen.atlassian.net/browse/NGSTACK-807