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

lightbox and forward #9027

Closed
hypeJunction opened this Issue Oct 12, 2015 · 10 comments

Comments

Projects
None yet
3 participants
@hypeJunction
Contributor

hypeJunction commented Oct 12, 2015

Not sure if there is an easy solution, but whenever a page handler uses forward(), lightbox receives and displays a json object. Too bad we use elgg_is_xhr() to assume Accept: application/json

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 12, 2015

Member

Does it do the same if you just return instead of forward()?

Member

mrclay commented Oct 12, 2015

Does it do the same if you just return instead of forward()?

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Oct 13, 2015

Contributor

Not really an option with forward('', '404') that is used all over the place

Contributor

hypeJunction commented Oct 13, 2015

Not really an option with forward('', '404') that is used all over the place

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 13, 2015

Member

Workaround: Make requests with a query string option or HTTP header that triggers unregistering ajax_forward_hook().

ajax_forward_hook() should be making sure ajax_action_hook() actually fired first. I.e. its logic shouldn't apply to non-actions. That might be a BC bugfix.

Member

mrclay commented Oct 13, 2015

Workaround: Make requests with a query string option or HTTP header that triggers unregistering ajax_forward_hook().

ajax_forward_hook() should be making sure ajax_action_hook() actually fired first. I.e. its logic shouldn't apply to non-actions. That might be a BC bugfix.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 13, 2015

Member

@ewinslow I assume in c327751 the intention was to send the JSON wrapper back only on actions?

Member

mrclay commented Oct 13, 2015

@ewinslow I assume in c327751 the intention was to send the JSON wrapper back only on actions?

@ewinslow

This comment has been minimized.

Show comment
Hide comment
@ewinslow

ewinslow Oct 21, 2015

Member

Yes

On Tue, Oct 13, 2015 at 7:21 AM Steve Clay notifications@github.com wrote:

@ewinslow https://github.com/ewinslow I assume in c327751
c327751
the intention was to send the JSON wrapper back only on actions?


Reply to this email directly or view it on GitHub
#9027 (comment).

Member

ewinslow commented Oct 21, 2015

Yes

On Tue, Oct 13, 2015 at 7:21 AM Steve Clay notifications@github.com wrote:

@ewinslow https://github.com/ewinslow I assume in c327751
c327751
the intention was to send the JSON wrapper back only on actions?


Reply to this email directly or view it on GitHub
#9027 (comment).

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Oct 22, 2015

Member

Next step: PR on 1.12 making sure ajax_forward_hook() does nothing if ajax_action_hook() was not previously called.

Member

mrclay commented Oct 22, 2015

Next step: PR on 1.12 making sure ajax_forward_hook() does nothing if ajax_action_hook() was not previously called.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Oct 23, 2015

Contributor

While at it, should probably do something about #8735

Contributor

hypeJunction commented Oct 23, 2015

While at it, should probably do something about #8735

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Dec 8, 2015

Contributor

I don't think we can fix this in a BC way. I am pretty sure I have seen people rely on json wrapper for non-action URLs.
One thing we could do is check the forward reason, and send a proper HTTP header for error pages. elgg.ajax should be able to interpret it as an error, and lightbox will fail silently.

Contributor

hypeJunction commented Dec 8, 2015

I don't think we can fix this in a BC way. I am pretty sure I have seen people rely on json wrapper for non-action URLs.
One thing we could do is check the forward reason, and send a proper HTTP header for error pages. elgg.ajax should be able to interpret it as an error, and lightbox will fail silently.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Dec 8, 2015

Contributor

Sort of a possible fix in #9210

Contributor

hypeJunction commented Dec 8, 2015

Sort of a possible fix in #9210

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Dec 8, 2015

Member

I have seen people rely on json wrapper for non-action URLs

Yep, as I'm documenting here, using forward() anywhere in an ajax request causes the JSON wrapper and output, ajax hook to be used.

Member

mrclay commented Dec 8, 2015

I have seen people rely on json wrapper for non-action URLs

Yep, as I'm documenting here, using forward() anywhere in an ajax request causes the JSON wrapper and output, ajax hook to be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment