EZP-31745: Introduce PagerSearchDataMapper interface

[ITernovtsii]

Question	Answer
Tickets	EZP-31745
Bug fix?	no
New feature?	no
BC breaks?	no
Tests pass?	no
Doc needed?	no
License	GPL-2.0

Currently, if I want to add some extra information in search result template, I can decorate
"Ibexa\Platform\Search\Mapper\PagerSearchContentToDataMapper" service.
But, decorator service is not accepted by SearchViewBuilder.

Checklist:

• Coding standards ($ composer fix-cs)

[ViniTou]

Also please rebase it and target it against 3.1 branch.

[ITernovtsii]

@ViniTou
Sorry, I don't see 3.1 branch here https://github.com/ezsystems/ezplatform-search/branches
I rebased it to 1.0.

[ViniTou]

@ezsystems/php-dev-team
As I totally agree with changing typehint for mapper to some abstraction, I now have mixed feelings about placing this interface here, or maybe rather with naming. If you look now, we already have \EzSystems\EzPlatformAdminUi\Tab\Dashboard\PagerContentToDataMapper which on first glance should also implement this interface but on other it is completely unrelated to Search.

[alongosz]

TL;DR;:

In this case I would suggest an event which will be dispatched inside the mapper to collect required extra parameters and attach them in each iteration or after the mapping, inside Search View Builder, to process an entire $view.

Review:

First of all, there are some out of scope whitespace changes, which makes it difficult to review. Not sure why you need them, but if you do, place them in a separate commit, please.

As I totally agree with changing typehint for mapper to some abstraction, I now have mixed feelings about placing this interface here, or maybe rather with naming. If you look now, we already have \EzSystems\EzPlatformAdminUi\Tab\Dashboard\PagerContentToDataMapper which on first glance should also implement this interface but on other it is completely unrelated to Search.

Since it uses separate namespaces, no strong feelings here. Could be PagerSearchDataMapper or language-wise more correct - but inconsistent - SearchPagerDataMapper. Notice that I didn't use Interface suffix here intentionally, because I consider it a bad extension of Hungarian notation and one of the worst practices advocated both by PSR and Symfony, for reasons passing my understanding.

Naming aside, using decoration here is very error prone and breaks a bit responsibility, because decorator would be required to provide all the necessary data items, including the core ones. This will lead to possible BC breaks, when our mapper gets updated with some new required parameters. We cannot give a BC promise on that, otherwise updating wouldn't be possible.

[ITernovtsii]

First of all, there are some out of scope whitespace changes, which makes it difficult to review. Not sure why you need them, but if you do, place them in a separate commit, please.

Ok, fix for yaml code style removed.

Since it uses separate namespaces, no strong feelings here. Could be PagerSearchDataMapper or language-wise more correct - but inconsistent - SearchPagerDataMapper. Notice that I didn't use Interface suffix here intentionally, because I consider it a bad extension of Hungarian notation and one of the worst practices advocated both by PSR and Symfony, for reasons passing my understanding.

Agree about naming) updated to "PagerSearchDataMapper".

using decoration here is very error prone and breaks a bit responsibility, because decorator would be required to provide all the necessary data items, including the core ones. This will lead to possible BC breaks, when our mapper gets updated with some new required parameters. We cannot give a BC promise on that, otherwise updating wouldn't be possible.

Sorry, but I didn't find any better way of extending this.
I used this in services.yaml

ContextualCode\EzPlatformAdminSearchBundle\Mapper\PagerSearchContentToDataMapper:
decorates: Ibexa\Platform\Search\Mapper\PagerSearchContentToDataMapper

and define class as:

use Ibexa\Platform\Search\Mapper\PagerSearchContentToDataMapper as BaseMapper;

class PagerSearchContentToDataMapper implements PagerSearchDataMapper
{
    /** @var BaseMapper */
    private $baseMapper;
    /** @var LocationService */
    private $locationService;
    /** @var BookmarkService */
    private $bookmarkService;

    public function __construct(
        BaseMapper $baseMapper,
        BookmarkService $bookmarkService,
        LocationService $locationService
    ) {
        $this->baseMapper = $baseMapper;
        $this->locationService = $locationService;
        $this->bookmarkService = $bookmarkService;
    }

    public function map(Pagerfanta $pager): array
    {
        $data = $this->baseMapper->map($pager);

        foreach ($data as $i => $result) {
            $data[$i]['location_is_bookmarked'] = $this->bookmarkService->isBookmarked(
                $this->locationService->loadLocation($result['mainLocationId'])
            );
        }

        return $data;
    }
}

This will work if the core mapper updated with some new parameters. It will break if mainLocationId removed from the core mapper, but I can live with this...
If you have any suggestions about how to better extend the mapper, I would appreciate it.

[alongosz]

In general 1.0 is no longer supported, rebase to 1.1 and fix other issues please.

[alongosz]

declare(strict_types=1);

[alongosz]

This is a BC break for this package. Any code which relied on PagerSearchContentToDataMapper is going to stop working. What you could do is to re-add this class in the form of PagerSearchContentToDataMapper extends PagerSearchContentToSearchDataMapper and annotate it as @deprecated since Ibexa v3.2, use PagerSearchContentToSearchDataMapper instead