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

With plugin enabled, onAdminSave event doesn't fire #58

Closed
mbirth opened this issue May 12, 2020 · 13 comments
Closed

With plugin enabled, onAdminSave event doesn't fire #58

mbirth opened this issue May 12, 2020 · 13 comments
Assignees
Labels
compat enhancement Enhancement for an existing feature fixed Issue has already been fixed

Comments

@mbirth
Copy link

mbirth commented May 12, 2020

Since flex-objects seems to replace the admin plugin's AdminController, the events fired there are not fired. So other plugins using onAdminSave and/or onAdminAfterSave won't work anymore.

@mahagr mahagr added compat enhancement Enhancement for an existing feature labels May 23, 2020
@mahagr
Copy link
Contributor

mahagr commented May 23, 2020

This is basically because of some plugins seem to break if I used the old events. I think we just need to add a few configuration options for the events.

@mahagr
Copy link
Contributor

mahagr commented Jul 1, 2020

Events were added in getgrav/grav@3a05dcf

@mahagr mahagr added the fixed Issue has already been fixed label Jul 1, 2020
@mahagr mahagr closed this as completed Jul 2, 2020
@NicoHood
Copy link
Contributor

I am not sure if this is 100% compatible.

The original code:
https://github.com/getgrav/grav-plugin-admin/blob/06c66794cc391f6d29b0261019b0a850cd0a0b84/classes/admincontroller.php#L666

The new:
getgrav/grav@3a05dcf#diff-01780a944f8c1d48066fed0f8b2391abe743e75a1b13a70ae9a4a938bd2a7a3cR243

I am not sure, I guess page must be renamed to object?

It seems those events are outdated anyways, so can you maybe point us to the new events that are able to modify the page correctly? Also is there an option to query for the grav version to subscribe to different events?

@mahagr
Copy link
Contributor

mahagr commented Dec 1, 2020

@NicoHood
Copy link
Contributor

NicoHood commented Dec 1, 2020

Oh, you are right! But what about Switching Flex Page object during onAdminSave event is not supported! Please update plugin., how to migrate that? What is the new event to use?

@mahagr
Copy link
Contributor

mahagr commented Dec 2, 2020

It means that you have replaced the page object in your plugin with another one. You shouldn't do that as it breaks the saving.

Instead of doing that, just modify the object before it gets saved.

@NicoHood
Copy link
Contributor

NicoHood commented Dec 2, 2020

Alright, so nothing will break. Thanks a lot for clarifying :-)

@NicoHood
Copy link
Contributor

NicoHood commented Dec 29, 2020

In my plugin I am checking the type of the object like this:

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

Now this does not work with the new flexobject. The check is still required, as saving a plugin config also fires this event.

But even if I ignore the check for now all page content is cleared on safe. This is possibly because I am modifying the page and this fails. But no error message is displayed this could be improved. I am still searching for a solution. Is there a common way to support Flexpages and "normal/old" Pages?

Edit: It seems setting page->header() causes trouble for me. It sets the frontmatter to something completely invalid:

"\0*\0items":
    "\0*\0items":
        title: uuuykuyk
        test: ok
        structured_data:
            place:
                name: egf
    "\0*\0nestedSeparator": .
    structured_data:
        place:
            name: Berlin
    '�*�items':
        title: uuuykuyk
        test: ok
        structured_data:
            place:
                name: egf
    '�*�nestedSeparator': .
"\0*\0nestedSeparator": .

You can reproduce with:

public function onAdminSave(Event $event)
{
    $page = $event['object'];
    
    // When saving plugin configurations we should ignore the event
    if (!($page instanceof \Grav\Common\Page\Interfaces\PageInterface)) {
        return;
    }
    
    $header = new Data((array)$page->header());
    $page->header($header->toArray());
}

The getter of header() returns wrong values, here is a json dump:

$this->grav['log']->emergency(json_encode((array)$page->header()));

{"\u0000*\u0000items":{"title":"ok","menu":"ok","test":"ok"},"\u0000*\u0000nestedSeparator":"."}

Is it possible that the header is now a Data object? I've requested that some time ago, but did not notice there was a change made in 1.7
It seems it is of type Grav\Common\Page\Header now.

This is my solution:

$header = $page->header();
if (!($header instanceof \Grav\Common\Page\Header)) {
    $header = new Data((array)$header);
}

@mahagr
Copy link
Contributor

mahagr commented Jan 4, 2021

Yes, check against PageInterface, it is common to all page types. Header object as toArray() method, imho.

@NicoHood
Copy link
Contributor

NicoHood commented Jan 5, 2021

Yeah, but the header is a breaking change that should be noted in the release notes! It is no object anymore it is Grav\Common\Page\Header now. Which makes a difference as you can see!

@mahagr
Copy link
Contributor

mahagr commented Jan 6, 2021

Header class has been there since 2015, but it was not used in the event before (not sure why). You should be using something like:

if (!$page instanceof PageInterface) {
    return;
}

$header = $page->header();
if (!$header instanceof Header) {
    $header = new Header((array)$header);
}

The above code should be working starting from Grav 1.6.

@mahagr
Copy link
Contributor

mahagr commented Jan 6, 2021

@mahagr
Copy link
Contributor

mahagr commented Jan 6, 2021

Also added it to changelog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compat enhancement Enhancement for an existing feature fixed Issue has already been fixed
Projects
None yet
Development

No branches or pull requests

3 participants