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

Isn't injected into UserDefinedForm pages #47

Open
purplespider opened this issue Aug 7, 2020 · 8 comments
Open

Isn't injected into UserDefinedForm pages #47

purplespider opened this issue Aug 7, 2020 · 8 comments

Comments

@purplespider
Copy link
Contributor

Better Navigator doesn't currently seem to work on UserDefinedForm pages.

Looks like the instance of DBHTMLText check is failing on these pages, so the HTML isn't getting injected:
https://github.com/jonom/silverstripe-betternavigator/blob/master/src/Extension/BetterNavigatorExtension.php#L54

That's as far as I managed to get in debugging it!

jonom added a commit that referenced this issue Aug 7, 2020
Ensure scripts and css are only loaded with navigator markup is present, and support HTTPResponse as input. Fixes #46, fixes #47
@jonom
Copy link
Owner

jonom commented Aug 7, 2020

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?

@purplespider
Copy link
Contributor Author

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.

@jonom
Copy link
Owner

jonom commented Aug 13, 2020

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!

@jonom
Copy link
Owner

jonom commented Aug 27, 2020

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.

@jonom
Copy link
Owner

jonom commented Aug 27, 2020

@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 🤷

@purplespider
Copy link
Contributor Author

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!

@jonom
Copy link
Owner

jonom commented Nov 20, 2020

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.

@purplespider
Copy link
Contributor Author

That sounds sensible to me, Jono! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants