EZP-22550 Added breadcrumb to navigate in Demobundle #87

StephaneDiot · 2014-05-14T11:55:19Z

Description

The goal here was to add a breadcrumb to be able to naviguate in the Demobundle. For that I implemented a new bundle from white october named BreadCrumbsBundle ( see link to whiteoctober bundle ).
To work this bundle need to be registred in ezPublishKernel.php and in composer.json ( see link to ezpublish-community pull request ).

Links

Jira : https://jira.ez.no/browse/EZP-22550
Whiteoctober BreadCrumbsBundle : https://github.com/whiteoctober/BreadcrumbsBundle
Ezpublish-community pull request link : ezsystems/ezpublish-community#150

Screenshot

dpobel · 2014-05-15T08:21:52Z

Controller/DemoController.php

+        $locationService = $this->getRepository()->getLocationService();
+        $path = $locationService->loadLocation( $locationId )->path;
+
+        for ( $i = 1; $i < count( $path ); $i++ )


the root location can be configured at the siteaccess level, so you should loop until the configured root location.

ezrobot · 2014-05-19T12:31:24Z

This Pull Request does not respect our PHP Coding Standards, please, see the report below:

FILE: ...undle-Pull-Request-code-sniffer/workspace/Controller/DemoController.php
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
 313 | ERROR | A space is required after opening parenthesis of function call
 313 | ERROR | A space is required before closing parenthesis of function call
 319 | ERROR | Expected "if (...)\n...{\n"; found "if (...) ...{\n"
 319 | ERROR | Expected 1 space before closing bracket; none found
 320 | ERROR | Expected "if (...)\n...{\n"; found "if(...)...{\n"
 328 | ERROR | Expected "}\nelse\n...{\n"; found "}            else ...{\n"
--------------------------------------------------------------------------------

StephaneDiot · 2014-05-19T12:31:34Z

Modifications done. ping @dpobel

ezrobot · 2014-05-19T12:36:20Z

This Pull Request does not respect our PHP Coding Standards, please, see the report below:

FILE: ...undle-Pull-Request-code-sniffer/workspace/Controller/DemoController.php
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
 319 | ERROR | Expected "if (...)\n...{\n"; found "if (...) ...{\n"
 320 | ERROR | Expected "if (...)\n...{\n"; found "if (...) ...{\n"
 327 | ERROR | Expected "}\nelse\n...{\n"; found "} else ...{\n"
--------------------------------------------------------------------------------

yannickroger · 2014-05-19T12:52:35Z

Not a single line of comment in your PR. The demo bundle is also here to tell people how to use the product. You should add some explanation here and there.

StephaneDiot · 2014-05-26T08:58:13Z

ping @yannickroger

andrerom · 2014-05-29T11:24:29Z

Controller/DemoController.php

+     * @param $locationId
+     * @return \Symfony\Component\HttpFoundation\Response
+     */
+    public function viewBreadcrumbAction ( $locationId )


"viewBreadcrumbAction (" -> "viewBreadcrumbAction("

hghandri · 2014-06-03T14:42:42Z

Hello,

How is managed cache to breadcrumb ? and how it is invalid if content is updated?

Thank.

ezrobot · 2014-06-03T15:26:24Z

This Pull Request does not respect our PHP Coding Standards, please, see the report below:

FILE: ...undle-Pull-Request-code-sniffer/workspace/Controller/DemoController.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 351 | ERROR | Expected "if (...)\n...{\n"; found "if(...)\n...{\n"
--------------------------------------------------------------------------------

bdunogier · 2014-06-04T09:00:45Z

Controller/DemoController.php

@@ -298,4 +298,62 @@ public function viewStarRatingAction( Location $location )
        );
    }

+    /**
+     * Displays breadcrumb for a given locationId


"For a given $locationId"

bdunogier · 2014-06-04T11:08:42Z

And please fix the commit messages: "EZP-22550 Added breadcrumb to navigate in Demobundle" (typo + removed "impl/".

bdunogier · 2014-06-04T11:08:58Z

What about caching, guys ?

As far as I can tell, it should be tweaked through smart cache rules, depending on the breadcrumb's max depth, but I might be missing something.

dpobel · 2014-06-04T12:37:00Z

Controller/DemoController.php

+        {
+            $location = $locationService->loadLocation( $path[$i] );
+            // if root location hasn't been found yet
+            if ( $isRootLocation == false )


nitpick: that's a bit weird, !$isRootLocation is easier to grasp (IMHO)

dpobel · 2014-06-04T12:42:16Z

besides the nitpick, it seems ok to me. For caching don't we need to use render_esi and to set the X-Location-Id header to the location id of the root location ?
ping @lolautruche

bdunogier · 2014-06-05T11:17:51Z

Well, the thing is that by default, I don't believe the cache rules will trigger cache clearing for a child location at depth 4 (it's just children by default, is it not ?).

In any case, it's the DemoBundle. We can take care of caching later (and I believe we should) and merge it as is.

lolautruche · 2014-06-05T12:25:35Z

Resources/config/services.yml

@@ -43,3 +43,5 @@ services:
        class: %ezdemo.form.type.feedback.class%
        tags:
            - { name: form.type, alias: ezdemo_feedback }
+
+    white_october_breadcrumbs: ~


This must not be here. This is semantic configuration so, should be either in ezpublish/config/config.yml or in a configuration file loaded by this bundle, like for ez_comments.yml.

lolautruche · 2014-06-05T12:37:44Z

As for caching, breadcrumbs are embedded in the main page and I'm not sure it worths it to use an ESI for that. So it would be expired with the current content.

andrerom · 2014-06-06T15:40:20Z

Wouldn't it be more correct, and more what legacy users would expect if it is esi, tied to menu root, so normal viewcache.ini tweaks applies? if tied to current content this is kind of not what user would expect at all, if viewcache.ini tied, it is more inline with how eZ Publish docs currently tells you to deal witch such cache.

lolautruche · 2014-06-09T07:29:18Z

@andrerom In ezdemo legacy, we have the following:

{cache-block keys=array( $module_result.uri, $user_hash, $extra_cache_key )}

So it's completely bound to the current URI (and therefore content), and user hash of course. So for me expectations should be the same.

andrerom · 2014-06-10T16:49:26Z

true, so +1 from here then
But if low hanging fruit to do it the more correct way straight away now on new stack, it is faster to do it now then let it flower in the backlog ;)

dpobel · 2014-06-11T08:33:04Z

on the cache thing: if the breadcrumb is tied to the page/content (ie not in an ESI), does not that mean that if for instance I rename the parent content, the breadcrumb will be wrong as the child pages cache will probably not be cleared ?

Given the cache-block example, I guess it's the same in legacy, so we can live with that for now, but this might be considered as a quite annoying bug.

So +1

lolautruche · 2014-06-11T09:18:02Z

Yes @dpobel you're right.

…_naviguate_in_DemoBundle EZP-22550 Added breadcrumb to navigate in Demobundle

StephaneDiot mentioned this pull request May 14, 2014

EZP-22550 Register wo BreadCrumbBundle and update dependencies for compo... ezsystems/ezpublish-community#150

Closed

dpobel reviewed May 15, 2014
View reviewed changes

andrerom reviewed May 29, 2014
View reviewed changes

StephaneDiot mentioned this pull request Jun 3, 2014

EZP-22250 Register wo breadcrumb bundle and update dependencies for composer ezsystems/ezpublish-community#162

Merged

bdunogier reviewed Jun 4, 2014
View reviewed changes

StephaneDiot changed the title ~~impl/EZP-22550 Added breadcrumb to naviguate in Demobundle~~ EZP-22550 Added breadcrumb to navigate in Demobundle Jun 4, 2014

dpobel reviewed Jun 4, 2014
View reviewed changes

lolautruche reviewed Jun 5, 2014
View reviewed changes

EZP-22550 Added breadcrumb to navigate in Demobundle

dbc7bee

dpobel added a commit that referenced this pull request Jun 18, 2014

Merge pull request #87 from StephaneDiot/EZP-22550_Impl_breadcrumb_to…

d4e52b9

…_naviguate_in_DemoBundle EZP-22550 Added breadcrumb to navigate in Demobundle

dpobel merged commit d4e52b9 into ezsystems:master Jun 18, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-22550 Added breadcrumb to navigate in Demobundle #87

EZP-22550 Added breadcrumb to navigate in Demobundle #87

StephaneDiot commented May 14, 2014

dpobel May 15, 2014

ezrobot commented May 19, 2014

StephaneDiot commented May 19, 2014

ezrobot commented May 19, 2014

yannickroger commented May 19, 2014

StephaneDiot commented May 26, 2014

andrerom May 29, 2014

hghandri commented Jun 3, 2014

ezrobot commented Jun 3, 2014

bdunogier Jun 4, 2014

bdunogier commented Jun 4, 2014

bdunogier commented Jun 4, 2014

dpobel Jun 4, 2014

dpobel commented Jun 4, 2014

bdunogier commented Jun 5, 2014

lolautruche Jun 5, 2014

lolautruche commented Jun 5, 2014

andrerom commented Jun 6, 2014

lolautruche commented Jun 9, 2014

andrerom commented Jun 10, 2014

dpobel commented Jun 11, 2014

lolautruche commented Jun 11, 2014

EZP-22550 Added breadcrumb to navigate in Demobundle #87

EZP-22550 Added breadcrumb to navigate in Demobundle #87

Conversation

StephaneDiot commented May 14, 2014

Description

Links

Screenshot

dpobel May 15, 2014

Choose a reason for hiding this comment

ezrobot commented May 19, 2014

StephaneDiot commented May 19, 2014

ezrobot commented May 19, 2014

yannickroger commented May 19, 2014

StephaneDiot commented May 26, 2014

andrerom May 29, 2014

Choose a reason for hiding this comment

hghandri commented Jun 3, 2014

ezrobot commented Jun 3, 2014

bdunogier Jun 4, 2014

Choose a reason for hiding this comment

bdunogier commented Jun 4, 2014

bdunogier commented Jun 4, 2014

dpobel Jun 4, 2014

Choose a reason for hiding this comment

dpobel commented Jun 4, 2014

bdunogier commented Jun 5, 2014

lolautruche Jun 5, 2014

Choose a reason for hiding this comment

lolautruche commented Jun 5, 2014

andrerom commented Jun 6, 2014

lolautruche commented Jun 9, 2014

andrerom commented Jun 10, 2014

dpobel commented Jun 11, 2014

lolautruche commented Jun 11, 2014