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

Make it possible to disable favicon using "serverside" http headers. … #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ComLock
Copy link

@ComLock ComLock commented Aug 9, 2016

…Typically done via checkbox and page controller.

@selbekk

…Typically done via checkbox and page controller.
@ComLock
Copy link
Author

ComLock commented Aug 9, 2016

Until I get this, my plan for a workaround is to run another response filter after the favicon filter and strip away the correct lines from res.pageContributions.headEnd.

Not very pretty, but it should work.

Hmm or I could simply compile and upload my version of the app to our Enonic XP installations.

@selbekk
Copy link
Owner

selbekk commented Aug 9, 2016

Hi there!

I don't quite understand the general use case here - do you want to remove the favicon if you're setting a header in the response from the server before this filter runs? Why?

return res;
}

if(res.headers && res.headers.serverSideDisableFavicon) {
delete res.headers.serverSideDisableFavicon; // Strip serverSide data so the end user never receives them.
}
Copy link
Owner

@selbekk selbekk Aug 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only way this code will ever run is if the imageId is not set (if you haven't uploaded an image yet).

If an image IS uploaded, the serverSideDisableFavicon header will be sent to the client after all (since it will return on line 8/11). You would either need to delete the header there as well, or do something like this:

var disableFavicon = res.headers && res.headers.serverSideDisableFavicon;
delete res.headers.serverSideDisableFavicon;
if (!imageId || disableFavicon) {
  return res;
}

var view = ..... // rest of code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are absolutely correct about that.

@ComLock
Copy link
Author

ComLock commented Aug 10, 2016

On some pages I want to disable the favicon.
For instance on pages used inside an iframe.
There is no point in having favicon multiple times on the same page.

The only way I know of to pass "data" from a page controller to a filter is via the response header.

See here:
https://discuss.enonic.com/t/passing-data-from-page-controller-to-responsefilter-via-http-response/609

@selbekk
Copy link
Owner

selbekk commented Aug 10, 2016

Ah, I get your use case. Is it really an issue though? Most pages included within iframes don't strip their favicons (if any) - and they'll just be ignored by the browser.

I made a quick codepen to check, and it looks like the browser doesn't download them at all.

Therefore, the only upside is saving about 500b of HTML - a pretty negligible amount. Measure that up with the complexity that goes with adding a header and then removing it again - a bug prone procedure in itself.

Thoughts?

@ComLock
Copy link
Author

ComLock commented Aug 10, 2016

I agree.

If Enonic decides to add some kind of server-side pass along data structure that would be better.

The only reason I "needed" to do it was a a third party that denied support because there was extra headers they did not specify. Nuff said...

@selbekk
Copy link
Owner

selbekk commented Aug 10, 2016

Ah getcha. Well, if you need to remove headers, you can simply create a new filter that removes any of your custom headers before sending the response.

@selbekk
Copy link
Owner

selbekk commented Aug 13, 2016

do you want me to close this pull request, or do you still want me to consider it?

@Bellfalasch
Copy link

This looks closable since the problem it solved (extra server request) wasn't a problem in reality (found out with the testing @selbekk did.

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.

3 participants