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

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

Closed
kiranetph opened this issue Jan 15, 2021 · 8 comments
Assignees
Labels
compat fixed Issue has already been fixed

Comments

@kiranetph
Copy link

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
Copy link
Contributor

mahagr commented Jan 15, 2021

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.

@mahagr mahagr self-assigned this Jan 15, 2021
@mahagr mahagr added the compat label Jan 15, 2021
@kiranetph
Copy link
Author

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
Copy link
Contributor

mahagr commented Jan 15, 2021

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:

  • call $pages->enablePages(); to initialize frontend (super slow and stupid logic which is why it's disabled)
  • use flex pages to grab the parent (it is always enabled in 1.7 and fast)

@kiranetph
Copy link
Author

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

@mahagr
Copy link
Contributor

mahagr commented Jan 16, 2021

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
Copy link
Contributor

mahagr commented Jan 16, 2021

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
Copy link
Author

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

@mahagr
Copy link
Contributor

mahagr commented Jan 28, 2021

Fixed in getgrav/grav@a35b9b1

@mahagr mahagr added the fixed Issue has already been fixed label Jan 28, 2021
@mahagr mahagr closed this as completed Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat fixed Issue has already been fixed
Projects
None yet
Development

No branches or pull requests

2 participants