trilbymedia / grav-plugin-flex-objects

Powerful and supremely flexible object support for Grav
MIT License
40 stars 10 forks source link

[v1.0.0-rc.20] onBlueprintCreated() called 4 times when Admin Pages is enabled #97

Closed kiranetph closed 3 years ago

kiranetph commented 3 years ago

Not sure if it is a bug or intended. I use the onBlueprintCreated() function in a plugin. When I enable Pages (Admin) it is called 4 times when opening a page in admin. 4times0

Under the old page it is only accessed 1 time.

 public function onBlueprintCreated(Event $event)
    {
        dump('Test');
mahagr commented 3 years ago

You're right, it does fire 4 times. It is not intentional, but may be required. Nonetheless, I will look into this when I have some time.

kiranetph commented 3 years ago

The problem here is the compatibility with "old pages", where it is called 1 x in the proper place. Under Flex objects Admin pages, only the 4th time, the $pages is correct, which seems too late to create the proper Blueprint. That is also the issue for #96

public function onBlueprintCreated(Event $event)
    {
        $blueprint = $event['blueprint'];  
        $pages = Grav::instance()['pages'];
        $route = '/' . ltrim(Grav::instance()['admin']->route, '/');
        $page         = $pages->find($route);     

        if ($page->parent()->template() == 'buttons'  ) {          
            $blueprints = new Blueprints(__DIR__ . '/blueprints/partials/');
            $extends = $blueprints->get('child-buttons');
            $blueprint->extend($extends, true);
        }      
    }
mahagr commented 3 years ago

Admin does not initialize frontend pages anymore. Some plugin may do that for you, which is why the last time works.

It is dangerous to use the method like this -- you may not be in admin at all or you may be outside of pages admin. In both cases you'd end to have bad blueprint. I cannot see the event working at all, it has too many flaws for it to be used like this.

In the mean time you can make it to work in two ways:

kiranetph commented 3 years ago

Thank you. I will first use $page->enablePages(); (it is working now) until I find out how to use flex pages. ;-)

mahagr commented 3 years ago

The following code should work everywhere (including Grav 1.6):

use Grav\Common\Plugin;
use Grav\Common\Flex\Types\Pages\PageObject;
use Grav\Common\Page\Page;
use Grav\Common\Page\Pages;
use Grav\Framework\Flex\Flex;
use Grav\Framework\Flex\FlexDirectory;
use Grav\Plugin\Admin\Admin;

// ...

class MyPlugin extends Plugin
{
   // ...

    public function onEventX(): void
    {
        // First check if you're in Pages Admin and editing a page.
        /** @var Admin|null $admin */
        $admin = $this->grav['admin'] ?? null;
        if (null === $admin || $admin->location !== 'pages' || ($key = trim($admin->route, '/')) === '') {
            return;
        }

        // Find the page, works both in the new Flex Pages and the old implementation.
        /** @var Flex|null $flex */
        $flex = $this->grav['flex'] ?? null;
        /** @var FlexDirectory|null $directory */
        $directory = $flex ? $flex->getDirectory('pages') : null;
        if (null !== $directory) {
            /** @var PageObject|null $page */
            $page = $directory->getObject($key);
            // Flex pages logic, remember to check $page !== null...
        } else {
            /** @var Pages $pages */
            $pages = $this->grav['pages'];
            /** @var Page|null $page */
            $page = $pages->find("/{$key}");
            // Old pages logic, remember to check $page !== null...
        }

        // Common logic for both types, remember to check $page !== null...
    }
}
mahagr commented 3 years ago

From the next version you can use something like this, too:

    public function onBlueprintCreated(Event $event)
    {
        /** @var \Grav\Plugin\FlexObjects\Flex $flex */
        $flex = $this->grav['flex_objects'] ?? null;
        $controller = $flex ? $flex->getAdminController() : null;
        $page = null;
        if ($controller) {
            $page = $controller->getObject();
            if ($page && $page->getFlexType() !== 'pages') {
                // We do not have a page.
                $page = null;
            }
        }

        // Do we need to fall back to the old logic?
        if (null === $page) {
            /** @var \Grav\Plugin\Admin\Admin|null $admin */
            $admin = $this->grav['admin'] ?? null;
            if ($admin && $admin->location === 'pages' && ($key = trim($admin->route, '/')) !== '') {
                /** @var \Grav\Common\Page\Pages $pages */
                $pages = $this->grav['pages'];
                /** @var \Grav\Common\Page\Page|null $page */
                $page = $pages->find("/{$key}");
            }
        }

        if (!$page instanceof \Grav\Common\Page\Interfaces\PageInterface) {
            return;
        }

        // Add your page logic here!
    }
kiranetph commented 3 years ago

Just to confirm that the $flex->getAdminController() approach for V1.7+ works as expected

mahagr commented 3 years ago

Fixed in https://github.com/getgrav/grav/commit/a35b9b12794466a0476dc8f66b709ab2cf687a7f