Skip to content
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

IBX-6906: [DX] Introduced identifier-based view matchers #322

Merged
merged 8 commits into from
Feb 5, 2024

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Jan 23, 2024

Question Answer
JIRA issue IBX-6906
Related to improves ibexa/dashboard#94
Required by ibexa/dashboard#95
Type improvement
Target Ibexa version v4.6+
BC breaks no
Regression run ibexa/commerce#614

The PR refactors ViewMatcherRegistry and ServiceAwareMatcherFactory to rely on identifier-based view matchers. Current behavior for service-name-based and class-name-based matchers is kept. To refer to a custom matcher using its identifier it's enough to add identifier attribute to its tag definition.

Before:

services:
    Ibexa\Bundle\Dashboard\ViewMatcher\IsDashboardMatcher:
        tags:
            - { name: ibexa.view.matcher }

ibexa:
    system:
        admin_group:
            content_view:
                dashboard:
                    dashboard_page:
                        template: '@ibexadesign/dashboard/dashboard_view.html.twig'
                        match:
                            '@Ibexa\Bundle\Dashboard\ViewMatcher\IsDashboardMatcher': true

After:

services:
    Ibexa\Bundle\Dashboard\ViewMatcher\IsDashboardMatcher:
        tags:
            - { name: ibexa.view.matcher, identifier: Ibexa\IsDashboard }

ibexa:
    system:
        admin_group:
            content_view:
                dashboard:
                    dashboard_page:
                        template: '@ibexadesign/dashboard/dashboard_view.html.twig'
                        match:
                            Ibexa\IsDashboard: true

Roads not taken

Identifier collision is not taken into the consideration as it's a bit of an overkill. That would require keeping the dropped compiler pass, copy prioritized tag sorting logic (that is given OOTB by tagged_iterator) and, for it to make sense, actually reverse sorted tag so it's possible to override matcher by using higher priority tagged service (so it's added later thus overriding existing one). We could consider keeping the compiler pass for validation purposes only. Instead, I propose that our matchers are prefixed with Ibexa\.

Side changes

  1. I had to introduce ViewMatcherRegistryInterface so final ViewMatcherRegistryInterface can be mocked for ServiceAwareMatcherFactory test case (the Registry was already covered).
  2. ClassNameMatcherFactory and its child ServiceAwareMatcherFactory $relativeNamespace constructor argument was changed to be strictly-typed as ?string. Anything else passed there would cause the code to crash, so this change is rather safe.
  3. ClassNameMatcherFactory::getMatcher returned \Ibexa\Core\MVC\Symfony\Matcher\ViewMatcherInterface while its child ServiceAwareMatcherFactory type-hinted \Ibexa\Core\MVC\Symfony\Matcher\ContentBased\MatcherInterface. This is clearly incorrect, so now both methods return base ViewMatcherInterface. It might lead to some 1st party static analysis issues, that would need to be fixed as well.

Future

In the future I'd like to completely get rid of ClassNameMatcherFactory in favor of identifier-based registered services, as that solution predates Dependency Injection Container, is confusing, and not very flexible. Making PHP FQCN prefix as a part of extension point proved to be not a very good idea, but probably the only sane one prior DIC.

QA affected scope

  1. Sanity of view matchers defined in https://doc.ibexa.co/en/master/templating/templates/view_matcher_reference/.
  2. Sanity of IsDashboard matcher introduced via ibexa/dashboard#94 and modified via ibexa/dashboard#95 PR

Documentation

View configuration section could be expanded to include specific configuration for the use case with identifier:

- To apply the matcher in view configuration, indicate the matcher by its fully qualified class name, preceded by \.
+ To use the matcher in view configuration, refer to it by its `identifier`
- To do it, create a matcher class that extends Ibexa\Core\MVC\Symfony\Matcher\ContentBased\MultipleValued.
+ To do it, create a matcher class that implements `\Ibexa\Core\MVC\Symfony\Matcher\ContentBased\MatcherInterface`.

The example needs to be replaced with the following one:

<?php

declare(strict_types=1);

namespace App\View\Matcher;

use Ibexa\Contracts\Core\Repository\UserService;
use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo;
use Ibexa\Contracts\Core\Repository\Values\Content\Location;
use Ibexa\Core\MVC\Symfony\Matcher\ContentBased\MatcherInterface;
use Ibexa\Core\MVC\Symfony\View\ContentValueView;
use Ibexa\Core\MVC\Symfony\View\LocationValueView;
use Ibexa\Core\MVC\Symfony\View\View;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;

class Owner implements MatcherInterface
{
    private UserService $userService;

    /** @var string[] */
    private array $matchingUserLogins;

    public function __construct(UserService $userService)
    {
        $this->userService = $userService;
    }

    /**
     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException
     */
    public function matchLocation(Location $location): bool
    {
        return $this->hasOwner($location->getContentInfo());
    }

    /**
     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException
     */
    public function matchContentInfo(ContentInfo $contentInfo): bool
    {
        return $this->hasOwner($contentInfo);
    }

    /**
     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException
     */
    public function match(View $view): ?bool
    {
        if ($view instanceof LocationValueView) {
            return $this->matchLocation($view->getLocation());
        }

        if ($view instanceof ContentValueView) {
            return $this->matchContentInfo($view->getContent()->contentInfo);
        }

        return false;
    }

    /**
     * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException
     */
    private function hasOwner(ContentInfo $contentInfo): bool
    {
        $owner = $this->userService->loadUser($contentInfo->ownerId);

        return in_array($owner->login, $this->matchingUserLogins, true);
    }

    /**
     * @param array<string> $matchingConfig
     */
    public function setMatchingConfig($matchingConfig): void
    {
        if (!is_array($matchingConfig)) {
            throw new InvalidArgumentException('App\Owner view matcher configuration has to be an array');
        }

        $this->matchingUserLogins = $matchingConfig;
    }
}
ibexa:
    system:
        site_group:
            content_view:
                full:
                    editor_articles:
                        template: '@ibexadesign/full/featured_article.html.twig'
                        match:
                            App\Owner: [johndoe, janedoe]

services:
    App\View\Matcher\Owner:
        autowire: true
        tags:
            - { name: ibexa.view.matcher, identifier: App\Owner }

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ibexa/engineering).

Copy link

sonarcloud bot commented Jan 23, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@alongosz alongosz added the Doc needed The changes require some documentation label Feb 5, 2024
@alongosz alongosz merged commit 2c8459d into main Feb 5, 2024
25 checks passed
@alongosz alongosz deleted the ibx-6906-view-matcher-identifier branch February 5, 2024 08:40
@julitafalcondusza julitafalcondusza removed the Doc needed The changes require some documentation label Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants