-
Notifications
You must be signed in to change notification settings - Fork 140
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
implement sitemap and error handling of seo-bundle #352
base: master
Are you sure you want to change the base?
Conversation
Ready to review now. |
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.
the build on travis keeps failing, can you please check that?
de: Fehlerseiten für SEO konforme Webseiten | ||
body: | ||
en: | | ||
TODO: translate |
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 says TODO - we should not merge without translation
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.
@dbu maybe you can give a hand for a good english translation?
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.
<li><a href="/en/company/mor">Error!!</a></li> | ||
</ul> | ||
fr: | | ||
TODO: translate |
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.
do we need french? i think just a few pages in french is enough to demo multilang. if we add more, we also have to maintain them
eine fehlerhafte URL hat? Zwei sog. SuggestionProvider | ||
helfen dabei mögliche Matches für Nachbarn der fehlerhaften URL-Eingabe oder, wenn vorhanden, einen | ||
darüber angeordneten Content anzuzeiten. <br /> | ||
Probiert es einfach aus, nehm buchstaben aus der aktuellen url oder probiert einden der folgenden links: |
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.
s/buchstaben/Buchstaben/
s/nehm/nimm/ (and i would change this to singular, so "Probier es einfach aus, nimm Buchstaben..."
s/einden/einen/
s/links/Links/
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 reveals something that I didn't discover before: Do we really need to have I don't like it that much. Can we maybe add a config option to whitelist FQCN of all documents that should be included in the sitemap? |
@wouterj from my POV i like interfaces more than configuration. In that case the interface forces to implement |
@ElectricMaxxx well, we can support both (decision is made using voters, isn't it?). This would result in the following decision flow for instance: |
Can we move that discussion and the visualization into an issue. I think it shouldn't stop the implementation of the current usage here, right?
Btw the way you painted it forces a user to do the decision twice. So what about having them side by side?
|
@@ -22,7 +22,7 @@ framework: | |||
twig: | |||
debug: '%kernel.debug%' | |||
strict_variables: '%kernel.debug%' | |||
exception_controller: 'FOS\RestBundle\Controller\ExceptionController::showAction' | |||
exception_controller: cmf_seo.error.suggestion_provider.controller:listAction |
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.
does this also handle encoding? like when i did a json request, do i get back a json error or a html page with error information?
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.
Not sure, need to have a look into the code or test it, would be nice.
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.
I propose to use the same strategy the TwigBundle uses: Use $request->getRequestFormat()
to determine the format. This way, we support all tools in the environment to determine the request format. (e.g. FOSRestBundle, custom things, _format
route placeholder, etc)
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.
but this should not have to happen here, right? is an issue of seo-bundle
|
||
<h3>Suggested pages</h3> | ||
|
||
{% for group, list in best_matches if list is not empty %} |
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.
please include the blue "docref" boxes we use throughout the sandbox:
<div class="alert alert-info clearfix">
<p>This page is rendered by the <code>SuggestionProviderController</code> of the CmfSeoBundle. This way, usefull suggestions can be shown to your users.</p>
<a class="docref" href="http://symfony.com/doc/current/cmf/bundles/seo/error_pages.html"><i class="glyphicon glyphicon-chevron-right"></i>Read about this feature in the CMF documentation.</a>
</div>
@@ -0,0 +1,32 @@ | |||
{% extends '::base.html.twig' %} |
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.
should be {% extends 'base.html.twig' %}
(same below)
enable_parent_provider: true | ||
enable_sibling_provider: true | ||
templates: | ||
html: ":error:index.html.twig" |
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.
please use error/index.html.twig
(same below)
Btw, I'm planning on restructuring the sandbox to put all feature demo's in under one |
@wouterj great idea. lets get this merged first, however. @ElectricMaxxx can you do the changes wouter asked for and then we merge? |
639d20d
to
f9fea5e
Compare
Apply fixes from StyleCI
DescriptionReadInterface, | ||
OriginalUrlReadInterface, | ||
KeywordsReadInterface | ||
class DemoSeoExtractor extends DemoSeoContent implements TitleReadInterface, DescriptionReadInterface, OriginalUrlReadInterface, KeywordsReadInterface |
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.
rather disable this fixer and keep this multiline to make it readable. put disabled: [single_line_class_definition]
into .styleci.yml
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.
oh. we should use that everywhere, cause i saw that on seo-bundle yesterday too. from which coding style is that?
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.
wouter added it in one of the bundles, can't remember which one
translate seo examples
Yea that PR misses the admins at the former places. So let's implement them first and rebase that one then. |
This PR implement the site map and error handling of seo-bundle.
It removes the
SandboxExceptionListener
and shows a nice error page. The sitmap can be found under the SEO stuff.