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

[RTM] Support news/events/faq in the breadcrumb module #186

Open
wants to merge 20 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@dmolineus
Copy link
Contributor

dmolineus commented Nov 15, 2018

Contao allows to define the news reader configuring the news archive. When using this setup, the news entry isn't added to the breadcrumb. This PR allows to add the news item as a new breadcrumb entry or overriding the current page breadcrumb item.

How it works:

  • A new option is added to the news archive where the breadcrumb mode can be defined
  • Supported options are override and extend
  • Depending on the setting a new breadcrumb item is created or the current page is adjusted
  • It only happens on the jumpTo page of the news archive and if an news entry is loaded by auto_item or items query parameter.

The current implementation assumes that overriding the current entry is the default and fallback behaviour. Disabling is not supported right now (but I don't mind to add it if wanted).

If you're happy with this solution, I'm willing to adapt this PR for events and faqs as well.

/cc @frontendschlampe

dmolineus added some commits Nov 15, 2018

@Toflar

This comment has been minimized.

Copy link
Member

Toflar commented Nov 15, 2018

Cool! I can't really comment on the feature itself because I haven't used it so far so I leave this up to the community and the other devs but what I can say is that the code quality is excellent and can be definitely considered to be merged. Once again: excellent work! 👍

@leofeyer leofeyer added the feature label Nov 15, 2018

@leofeyer leofeyer added this to the 4.7.0 milestone Nov 15, 2018

@asaage

This comment has been minimized.

Copy link

asaage commented Nov 15, 2018

nice...
this is related to:

in case #161 gets merged you might consider to
return $news->pageTitle;
instead of
return $news->headline;
inside the getNewsTitle function.?!

Looking forward to have this in events and faqs 👍

@dmolineus

This comment has been minimized.

Copy link
Contributor

dmolineus commented Nov 15, 2018

in case #161 gets merged you might consider to
return $news->pageTitle;

Thanks for the hint. At the moment I don't add it. Maybe adding a public function getPageTitle(): string to NewsModel would be nice which returns the pageTitle and uses the headline as fallback.

dmolineus added some commits Nov 15, 2018

@dmolineus dmolineus force-pushed the dmolineus:feature/news-breadcrumb branch from f4192ed to 3ea467e Nov 15, 2018

@dmolineus dmolineus changed the title Add configuration to append news to breadcrumb [RFC] Add configuration to append news to breadcrumb Nov 15, 2018

dmolineus added some commits Nov 20, 2018

Let's gonna play with magic: Use magic method calls instead of explic…
…it __call method calls for CS reasons.

@dmolineus dmolineus changed the title [RFC] Add configuration to append news to breadcrumb [RFC] Support news/events/faq in the breadcrumb module Nov 22, 2018

@dmolineus

This comment has been minimized.

Copy link
Contributor

dmolineus commented Nov 22, 2018

I added support for calendar events and faq entries as well. The implementation for FAQ entries only works if a jumpTo page is defined (not required so far). From my point of view, it's ready to merge now.

@dmolineus dmolineus changed the title [RFC] Support news/events/faq in the breadcrumb module [FTM] Support news/events/faq in the breadcrumb module Nov 22, 2018

@dmolineus dmolineus changed the title [FTM] Support news/events/faq in the breadcrumb module [RTM] Support news/events/faq in the breadcrumb module Nov 22, 2018

@leofeyer leofeyer force-pushed the contao:master branch 2 times, most recently from cb069f4 to 10ba742 Dec 4, 2018

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 7, 2018

Just so I understand the purpose of this PR; this is how the breadcrumb looks like:

Contao Official Demo > Modules > Events > Event Detail

Now you want to add variant A (override):

Contao Official Demo > Modules > Events > Contao North-Day

And variant B (extend):

Contao Official Demo > Modules > Events > Event Detail > Contao North-Day

Correct?

@dmolineus

This comment has been minimized.

Copy link
Contributor

dmolineus commented Dec 7, 2018

Exactly, that's that the purpose of this PR. Variant B is useful if the the reader is on the same page as the list module. Variant A is useful if a detail page is used.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 7, 2018

Hm, but variant B is actually wrong, because the Event Detail page will result in a 404 error if called without an item (see contao/core#8361), therefore we should never link to it.

And variant A is already possible by replacing <?= $item['link'] ?> with {{page::pageTitle}} in the mod_breadcrumb.html5 template.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 7, 2018

I see how variant B is correct if the reader is on the same page as the list module. But it can easily be misconfigured for sure.

Does it need to be configurable at all? Can we not just apply the correct mode automatically?

@dmolineus

This comment has been minimized.

Copy link
Contributor

dmolineus commented Dec 7, 2018

Hm, but variant B is actually wrong, because the Event Detail page will result in a 404 error if called without an item (see contao/core#8361), therefore we should never link to it.

It's designed for the case that the reader is on the same page as the list/archive module. If you have a separate reader page the override mode is the way to go.

And variant A is already possible by replacing with {{page::pageTitle}} in the mod_breadcrumb.html5 template.

Sure, but then you aren't able to used both modes in your installation.

Does it need to be configurable at all? Can we not just apply the correct mode automatically?

It would be great but I don't know how. There are many ways to include the frontend modules. So I don't know how to check which mode should be used automatically. Detecting in in the reader/list module is too late as the breadcrumb might be rendered before. But maye you have a good idea how to solve it.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 7, 2018

Maybe we can use the new $objPage->requireItem setting? We extend the breadcrumb by default except if the page requires an item, in which case we override the breadcrumb (because the link to the page without item would result in an 404 error).

@dmolineus

This comment has been minimized.

Copy link
Contributor

dmolineus commented Dec 7, 2018

Maybe we can use the new $objPage->requireItem setting? We extend the breadcrumb by default except if the page requires an item, in which case we override the breadcrumb (because the link to the page without item would result in an 404 error).

Sounds reasonable and should work. Didn't know about that setting before. I could update this PR for this next week.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 7, 2018

Thank you.

Didn't know about that setting before.

It is a new feature in Contao 4.6, so probably not very known yet.

dmolineus added some commits Dec 7, 2018

Use $objPage->requireItem instead of configuration to determine if br…
…eadcrumb should be added or overridden.
@dmolineus

This comment has been minimized.

Copy link
Contributor

dmolineus commented Dec 10, 2018

I adjusted this pull request so that it works now without any further customization. It depends on the $GLOBALS['objPage']->requireItem setting now. Maybe there should be any hint now so that users know how to configure it.

One thing that might be changed is the usage of the lately introduced page title setting in news and events. @leofeyer Should I add it to this PR as well?

@fritzmg

This comment has been minimized.

Copy link
Contributor

fritzmg commented Dec 10, 2018

I adjusted this pull request so that it works now without any further customization. It depends on the $GLOBALS['objPage']->requireItem setting now.

In Contao 4.7 you can now automatically replace the news list with a news reader, when there is an item present - just as with event lists. This means you must not enable the requireItem option. How does the bread crumb behave in that case?

@dmolineus

This comment has been minimized.

Copy link
Contributor

dmolineus commented Dec 10, 2018

In Contao 4.7 you can now automatically replace the news list with a news reader, when there is an item present - just as with event lists. This means you must not enable the requireItem option. How does the bread crumb behave in that case?

There are two cases: If requireItem is enabled the breadcrumb entry of the current page is overriden. Otherwise a new entry for the news/faq/event is added.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Dec 11, 2018

One thing that might be changed is the usage of the lately introduced page title setting in news and events. @leofeyer Should I add it to this PR as well?

If you mean 'title' => \StringUtil::specialchars($news->pageTitle ?: $news->headline, true) then yes, please add this in your PR as well. 👍

dmolineus added some commits Dec 18, 2018

@dmolineus

This comment has been minimized.

Copy link
Contributor

dmolineus commented Dec 18, 2018

If you mean 'title' => \StringUtil::specialchars($news->pageTitle ?: $news->headline, true) then yes, please add this in your PR as well. +1

Yes, exactly. It's added new with 2132c70.

private function getEventAlias(): ?string
{
if (!isset($GLOBALS['objPage'])) {

This comment has been minimized.

@leofeyer

leofeyer Jan 9, 2019

Member

When does this case occur?

This comment has been minimized.

@dmolineus

dmolineus Jan 9, 2019

Contributor

For instance, if the breadcrumb module is loaded via an esi request which hasn't setup the page context. In general it's a global state outside of the control of this listener. In the sense of defensive programming, I check it here.

This comment has been minimized.

@leofeyer

leofeyer Jan 10, 2019

Member

@Toflar Is this possible?

This comment has been minimized.

@Toflar

Toflar Jan 10, 2019

Member

Guess so. Defensive programming 👍

$configAdapter = $this->framework->getAdapter(Config::class);
if ($configAdapter->get('useAutoItem')) {
return $inputAdapter->get('auto_item');

This comment has been minimized.

@leofeyer

leofeyer Jan 9, 2019

Member

This is already done here and does not need to be done again – except it you want to use the Symfony request instead of the Input class, which we should probably do.

This comment has been minimized.

@dmolineus

dmolineus Jan 9, 2019

Contributor

Only, if the news reader is loaded before the breadcrumb module. I don't want to limit it to this precondition.

This comment has been minimized.

@leofeyer

leofeyer Jan 10, 2019

Member

Ah, no, this assumption is wrong. Only the EventReader class can be sure that auto_item is supposed to be mapped to events. Right now, the listener adds an event breadcrumb item as soon as any auto_item parameter is present, without even knowing if the auto_item is related to an event!

Only the EventReader can assume: "If I am called, there should be an events parameter. If there is no events parameter but I am still called, I should probably look for an auto_item parameter."

Therefore the BreadcrumbListener must only check the events parameter – even if this means that the feature will only work if the breadcrumb menu is loaded after the event reader.

This comment has been minimized.

@dmolineus

dmolineus Jan 10, 2019

Contributor

Yes, I know that this is a limitation. However, it depends on the jumpTo url of the news/event/faq. Afaik it's not possible to multiple reader modules on a page. That's why the current solution makes sense for me.

If multiple reader pages are mapped to one page, there will be a wrong behaviour. But then you would have other issues as well.

This comment has been minimized.

@leofeyer

leofeyer Jan 10, 2019

Member

Afaik it's not possible to multiple reader modules on a page.

Of course it is. You can have a news reader and an event reader on the same page and trigger one or another with the items or events parameter.

This comment has been minimized.

@dmolineus

dmolineus Jan 10, 2019

Contributor

Of course it is. You can have a news reader and an event reader on the same page and trigger one or another with the items or events parameter.

Ah, I see. I was refering about using auto_item didn't thought about using the proper parameters.

This comment has been minimized.

@dmolineus

dmolineus Jan 17, 2019

Contributor

Ah, no, this assumption is wrong. Only the EventReader class can be sure that auto_item is supposed to be mapped to events. Right now, the listener adds an event breadcrumb item as soon as any auto_item parameter is present, without even knowing if the auto_item is related to an event!

I have thought about it again and I don't see what's the problem here.

How the reader modules work

  1. The reader modules loads a model regarding the items (faq, news) or events (events) parameter. For simplicity I talk only about the items parameter later on.
  2. If the useAutoItem is defined the auto_item is mapped to the items parameter in the reader module.
  3. If the reader module has a defined items parameter and does not find any published model, it will throw an PageNotFoundException exception.
  4. If no items parameter is defined, an empty string is returned.
  5. If two reader modules of different types are on the same page, it would only work if the event reader is combined with news or faq. Otherwise a module would trigger the PageNotFoundException exception.

How this PR would work

  1. The breadcrumb listeners checks if the current page is defined as jumpTo page in the news archive / calendar / faq category setting
  2. If this is true, it detects the items parameter regarding the auto_item setting
  3. It try to find the model
  4. If an model exists it would append or override the breadcrumb item regarding requireItem setting in the current page.

Right now, the listener adds an event breadcrumb item as soon as any auto_item parameter is present, without even knowing if the auto_item is related to an event!

There are only two scenarios where this would become true:

  1. useAutoItem is deactivated and news and faq reader is on the same page.
  2. useAutoItem is activated and two reader modules are on the same page.

Both scenarios have the issue: As soon as any auto_item is not found, a PageNotFoundException exception would be thrown. Only if they match the mentioned issue would occur.

Is it a real project issue? Who uses two reader modules on the same page ensuring identical aliases for each published news/event/faq presented on the page?

Show resolved Hide resolved calendar-bundle/src/EventListener/BreadcrumbListener.php Outdated
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 10, 2019

This is how I would simplify the BreadcrumbListener class:

<?php

declare(strict_types=1);

/*
 * This file is part of Contao.
 *
 * (c) Leo Feyer
 *
 * @license LGPL-3.0-or-later
 */

namespace Contao\CalendarBundle\EventListener;

use Contao\CalendarEventsModel;
use Contao\CalendarModel;
use Contao\CoreBundle\Framework\ContaoFrameworkInterface;
use Contao\Events;
use Contao\Input;
use Contao\PageModel;
use Contao\StringUtil;

class BreadcrumbListener
{
    /**
     * @var ContaoFrameworkInterface
     */
    private $framework;

    public function __construct(ContaoFrameworkInterface $framework)
    {
        $this->framework = $framework;
    }

    public function onGenerateBreadcrumb(array $items): array
    {
        if (!isset($GLOBALS['objPage']) || !$eventModel = $this->getEventModel()) {
            return $items;
        }

        if ($GLOBALS['objPage']->requireItem) {
            return $this->overrideActiveBreadcrumbItem($items, $eventModel);
        }

        return $this->addBreadcrumbItem($items, $eventModel);
    }

    private function getEventModel(): ?CalendarEventsModel
    {
        if (isset($_GET['events'])) {
            return null;
        }

        /** @var Input $inputAdapter */
        $inputAdapter = $this->framework->getAdapter(Input::class);

        if (!$eventAlias = $inputAdapter->get('events')) {
            return null;
        }

        /** @var CalendarModel $calendarAdapter */
        $calendarAdapter = $this->framework->getAdapter(CalendarModel::class);

        if (!$calendarModel = $calendarAdapter->findOneByJumpTo($GLOBALS['objPage']->id)) {
            return null;
        }

        /** @var CalendarEventsModel $calendarEventsAdapter */
        $calendarEventsAdapter = $this->framework->getAdapter(CalendarEventsModel::class);

        return $calendarEventsAdapter->findPublishedByParentAndIdOrAlias($eventAlias, [$calendarModel->id]);
    }

    private function addBreadcrumbItem(array $items, CalendarEventsModel $eventModel): array
    {
        $currentPage = $this->getCurrentPage();

        foreach ($items as &$item) {
            $item['isActive'] = false;
        }

        unset($item);

        /** @var Events $events */
        $events = $this->framework->getAdapter(Events::class);
        $title = $this->getEventTitle($eventModel, $currentPage);

        $items[] = [
            'isRoot' => false,
            'isActive' => true,
            'href' => $events->generateEventUrl($eventModel),
            'title' => StringUtil::specialchars($title, true),
            'link' => $title,
            'data' => $currentPage->row(),
            'class' => ''
        ];

        return $items;
    }

    private function overrideActiveBreadcrumbItem(array $items, CalendarEventsModel $eventModel): array
    {
        $currentPage = $this->getCurrentPage();

        /** @var Events $events */
        $events = $this->framework->getAdapter(Events::class);
        $title = $this->getEventTitle($eventModel, $currentPage);

        foreach ($items as &$item) {
            if ($item['isActive'] && $item['data']['id'] === $currentPage->id) {
                $item['href'] = $events->generateEventUrl($eventModel);
                $item['title'] = StringUtil::specialchars($title, true);
                $item['link'] = $title;

                break;
            }
        }

        return $items;
    }

    /**
     * Re-fetches the page model in case the title of the global page object has already been overwritten.
     */
    private function getCurrentPage(): PageModel
    {
        /** @var PageModel $pageAdapter */
        $pageAdapter = $this->framework->getAdapter(PageModel::class);

        if (!$pageModel = $pageAdapter->findByPk($GLOBALS['objPage']->id)) {
            throw new \RuntimeException('The ID of the global page object is invalid');
        }

        return $pageModel;
    }

    private function getEventTitle(CalendarEventsModel $eventModel, PageModel $currentPage): string
    {
        if ($eventModel->pageTitle) {
            return $eventModel->pageTitle;
        }

        if ($eventModel->title) {
            return $eventModel->title;
        }

        return $currentPage->pageTitle ?: $currentPage->title;
    }
}

@leofeyer leofeyer modified the milestones: 4.7.0, 4.8.0 Jan 16, 2019

dmolineus added some commits Jan 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment