implement sitemap and error handling of seo-bundle by ElectricMaxxx · Pull Request #352 · symfony-cmf/cmf-sandbox

ElectricMaxxx · 2016-10-27T20:06:27Z

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.

ElectricMaxxx · 2016-10-27T20:24:49Z

Ready to review now.

dbu

the build on travis keeps failing, can you please check that?

dbu · 2016-10-29T09:07:04Z

+          de: Fehlerseiten für SEO konforme Webseiten
+      body:
+          en: |
+              TODO: translate


this says TODO - we should not merge without translation

@dbu maybe you can give a hand for a good english translation?

dbu · 2016-10-29T09:07:48Z

+                <li><a href="/en/company/mor">Error!!</a></li>
+              </ul>
+          fr: |
+              TODO: translate


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

dbu · 2016-10-29T09:09:04Z

+              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:


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/

wouterj · 2016-10-29T10:06:31Z

This reveals something that I didn't discover before: Do we really need to have SitemapAwareInterface in all documents that we want to display in the sitemap?

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?

ElectricMaxxx · 2016-10-29T11:04:00Z

@wouterj from my POV i like interfaces more than configuration. In that case the interface forces to implement isVisibleOnSitemap(), which is more than a decider for true false. You can decide based on your application/bussiness logic there. So a document can be on a sitemap in one case and not for the other case. And an other point for the interface is the chance to use different named sitemaps (is implemented) So the decider can vote on a special sitemap.

wouterj · 2016-10-29T11:53:39Z

@ElectricMaxxx well, we can support both (decision is made using voters, isn't it?). This would result in the following decision flow for instance:

ElectricMaxxx · 2016-10-29T12:36:55Z

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?

dbu · 2016-11-07T16:51:08Z

    debug:            '%kernel.debug%'
    strict_variables: '%kernel.debug%'
-    exception_controller: 'FOS\RestBundle\Controller\ExceptionController::showAction'
+    exception_controller: cmf_seo.error.suggestion_provider.controller:listAction


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?

Not sure, need to have a look into the code or test it, would be nice.

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)

but this should not have to happen here, right? is an issue of seo-bundle

wouterj · 2016-11-08T13:51:02Z

+
+    <h3>Suggested pages</h3>
+
+    {% for group, list in best_matches if list is not empty %}


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>

wouterj · 2016-11-08T13:51:50Z

@@ -0,0 +1,32 @@
+{% extends '::base.html.twig' %}


should be {% extends 'base.html.twig' %} (same below)

wouterj · 2016-11-08T13:52:19Z

+         enable_parent_provider: true
+         enable_sibling_provider: true
+         templates:
+             html: ":error:index.html.twig"


please use error/index.html.twig (same below)

wouterj · 2016-11-08T13:54:08Z

Btw, I'm planning on restructuring the sandbox to put all feature demo's in under one Feature page. The extra benefit is that it would make the hierarchy a lot complexer (and thus a nicer demo).

dbu · 2016-11-09T08:19:57Z

@wouterj great idea. lets get this merged first, however.

@ElectricMaxxx can you do the changes wouter asked for and then we merge?

Apply fixes from StyleCI

dbu · 2016-12-13T07:29:29Z

-    DescriptionReadInterface,
-    OriginalUrlReadInterface,
-    KeywordsReadInterface
+class DemoSeoExtractor extends DemoSeoContent implements TitleReadInterface, DescriptionReadInterface, OriginalUrlReadInterface, KeywordsReadInterface


rather disable this fixer and keep this multiline to make it readable. put disabled: [single_line_class_definition] into .styleci.yml

oh. we should use that everywhere, cause i saw that on seo-bundle yesterday too. from which coding style is that?

wouter added it in one of the bundles, can't remember which one

translate seo examples

ElectricMaxxx · 2016-12-13T08:41:25Z

Yea that PR misses the admins at the former places. So let's implement them first and rebase that one then.

ElectricMaxxx added the wip/poc label Oct 27, 2016

dbu suggested changes Oct 29, 2016

View reviewed changes

dbu reviewed Nov 8, 2016

View reviewed changes

wouterj reviewed Nov 8, 2016

View reviewed changes

dbu approved these changes Nov 9, 2016

View reviewed changes

ElectricMaxxx added 8 commits December 12, 2016 21:23

implement sitemap and error handling of seo-bundle

f7841da

changes due to hints of styleCI

42475f6

re-add former error text and information

c1a67a2

test for error page

482cfc1

add error description page

32764d2

remove french translation

b566cf7

fix ci test

11714cb

fix issues due to hints

f9fea5e

ElectricMaxxx force-pushed the seo_sitemap_and_error_handling branch from 639d20d to f9fea5e Compare December 13, 2016 05:36

ElectricMaxxx added 2 commits December 13, 2016 05:40

Apply fixes from StyleCI

ae1ba37

Merge pull request #355 from symfony-cmf/analysis-8wkK9e

99620d1

Apply fixes from StyleCI

dbu reviewed Dec 13, 2016

View reviewed changes

dbu and others added 2 commits December 13, 2016 08:34

translate seo examples

42081c0

Merge pull request #356 from symfony-cmf/seo-translations

19dea4f

translate seo examples


		<h3>Suggested pages</h3>

		{% for group, list in best_matches if list is not empty %}

Conversation

ElectricMaxxx commented Oct 27, 2016

Uh oh!

ElectricMaxxx commented Oct 27, 2016

Uh oh!

dbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commented Oct 29, 2016

Uh oh!

ElectricMaxxx commented Oct 29, 2016

Uh oh!

wouterj commented Oct 29, 2016

Uh oh!

ElectricMaxxx commented Oct 29, 2016 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wouterj commented Nov 8, 2016

Uh oh!

dbu commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElectricMaxxx commented Dec 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

wouterj Nov 8, 2016 •

edited

Loading

wouterj Nov 8, 2016 •

edited

Loading