Isn't injected into UserDefinedForm pages about silverstripe-betternavigator HOT 8 OPEN

Hi @purplespider looks like #46 is related. I pushed up a fix a while ago (tweaked it this morning) that I think should help you - would you be able to try this branch and report back if it solves the problem for you?

from silverstripe-betternavigator.

Thanks, Jono. just tried the fix-46 branch, and while you no longer get a JS error in the console on User Form pages, Better Navigator still doesn't appear. Displays on other pages fine.

Tested on fresh Silverstripe 4.6.0 and latest UserForms 5.6.0.

from silverstripe-betternavigator.

Interesting. I don't use UserForms myself so I may not get a chance to debug this further for now, sorry. If you're able to look in to it more let me know what you find!

from silverstripe-betternavigator.

I went further down the rabbit hole this morning. Problem seems to be that the result of Controller::handleAction() can be all kinds of things, including a Controller (API says only DBHTMLText|HTTPResponse but not true). This is accounted for later in the pipeline - Controller::handleRequest() calls Controller::prepareResponse which is responsible for rendering Controllers and things out to HTML. I guess we could replicate that functionality within BetterNavigatorExtension but it feels like overkill and probably doubles render effort.

The API documentation is pretty far off here, like RequestHandler::handleAction() says they only thing it can return is a HTTPResponse, but that conflicts with reality and comments in the code.

I don't see an extension point we can use at an appropriate point in the response pipeline to solve this issue but maybe there's a middleware we can use to check for a HTML response really late in the pipeline? @unclecheese would really value your input here. (Side note: this is why I insisted on tagging a major release for that seemingly innocent change 😅)

@purplespider can you please try installing v4 instead of v5 and see if that fixes the problem for you. You'll need to put $BetterNavigator in your main page template but otherwise it's almost the same as v5.

from silverstripe-betternavigator.

@purplespider possibly just changing UserForms to render the result would fix the issue for you. Change this part to

return $this->render([
    'Content' => DBField::create_field('HTMLText', $this->Content),
    'Form' => $this->Form()
]);

I guess that would probably constitute a breaking change though 🤷

from silverstripe-betternavigator.

Thanks for looking into this further, Jono.

@purplespider can you please try installing v4 instead of v5 and see if that fixes the problem for you. You'll need to put $BetterNavigator in your main page template but otherwise it's almost the same as v5.

Yep, I can confirm that v4 works fine on UserForms pages.

@purplespider possibly just changing UserForms to render the result would fix the issue for you. Change this part to

return $this->render([
    'Content' => DBField::create_field('HTMLText', $this->Content),
    'Form' => $this->Form()
]);

I guess that would probably constitute a breaking change though 🤷

Yep, this makes v5 work!

from silverstripe-betternavigator.

I think the solution for this is going to be re-adding support for $BetterNavigator in templates, but making it optional, so sites that are affected by this issue (e.g. any using UserForms) have a way to fix it, but otherwise it's unnecessary.

In the afterCallActionHandler we can probably do a simple strpos test to see if the BetterNavigator HTML or script has already been added, and skip adding it again if so.

from silverstripe-betternavigator.

That sounds sensible to me, Jono! Thanks.

from silverstripe-betternavigator.