Skip to content

Lower hook priority#2

Closed
gmclelland wants to merge 2 commits intowanze:masterfrom
gmclelland:patch-1
Closed

Lower hook priority#2
gmclelland wants to merge 2 commits intowanze:masterfrom
gmclelland:patch-1

Conversation

@gmclelland
Copy link
Copy Markdown
Contributor

Hi Wanze,

I'm not sure if this will fix the problem, but I've noticed some other modules that add markup using a Page::render hook have to raise their hook's priority above TemplateEngineFactory's priority to make sure their hook has a higher priority.

I believe in both cases the modules were outputting html right before the closing body tag using the Page::render hook.

I ran into this problem with Tracy Debugger and https://github.com/madebymats/FieldtypeLeafletMapMarker.

Tracy Debugger is fixed now by raising it's hook priority really high. I have a pull request FriendsOfProcessWire/FieldtypeLeafletMapMarker#9 to fix it for FieldtypeLeafletMapMarker, but should all other modules have to do this? Can TemplateEngineFactory lower it's priority so other modules won't have to raise their priority?

Here's what it looks like on my site:
image

Notice how the MarkupAddInlineScript module (100.1) runs before TemplateEngineTwig (100.3)? In this case the Leaflet initialization javascript markup doesn't get added before the closing body tag.

The only thing I'm not sure of is if it is ok to go under 100 which is the default priority?

Maybe this module should set it to 100.1? I'm not sure. Maybe you know more about this?

Hi Wanze,

I'm not sure if this will fix the problem, but I've noticed some other modules that add markup using a hook have to raise their hook's priority to make sure their hook has a higher priority.

I ran into this problem with Tracy Debugger and https://github.com/madebymats/FieldtypeLeafletMapMarker.

Tracy Debugger is fixed now by raising it priority really high.  I have a pull request FriendsOfProcessWire/FieldtypeLeafletMapMarker#9 to fix it for FieldtypeLeafletMapMarker, but should all other modules have to do this?  Can TemplateEngineFactory lower it's priority so other modules won't have to raise their priority?

Here's what it looks like on my site:
@wanze
Copy link
Copy Markdown
Owner

wanze commented Feb 27, 2017

@gmclelland Thanks for your pull request. Does it work on your environment if you decrease the priority to 99? I had a similar problem with this module in combination with ProCache. Setting the priority to less than 100 didn't work because Page::render() itself is added via Hook. See ryancramerdesign/ProcessWire#1218 for more details.

If it works, I'm happy to merge, otherwise we need another solution.

Cheers

@gmclelland
Copy link
Copy Markdown
Contributor Author

It works for me with '100.01' but it doesn't work when you set it to '100.1'

image

Updated the priority.  Works for me.
@gmclelland
Copy link
Copy Markdown
Contributor Author

HI @wanze, I updated the Pull Request to 100.01 which should be safe and works in my testing.

What your thoughts on this? Does it look ok to you?

@wanze
Copy link
Copy Markdown
Owner

wanze commented Mar 2, 2017

@gmclelland Thank you, looks good. Actually I'm surprised that you can pass "float strings" as priority, I think the description should be updated ;-) Could you open another pull request targeting the dev branch of this repository? I will merge the PR, do some tests, update version number and merge into master. Thanks again :)

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

Successfully merging this pull request may close these issues.

2 participants