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

Fix broken AJAX URLs #50

Merged
merged 1 commit into from
Oct 22, 2018
Merged

Conversation

carlalexander
Copy link
Contributor

This is a fix for an issue with the rewrite rules for admin-ajax.php. You cannot create rewrite rules this way as this isn't how rewrite rules in WordPress work. (You can read more about it here.) These rewrite rules might work with Apache but only because they're added to the .htaccess file. They won't work with nginx.

Instead, the correct way to do this is to use the ajaxurl variable that WordPress puts at your disposition. I've edited amber.js to make use of it. I've also removed the rewrite rules. There was also a missing AJAX action for logcacheview which I added.

@jlicht jlicht merged commit 5aa7610 into berkmancenter:wordpress Oct 22, 2018
@jlicht
Copy link
Collaborator

jlicht commented Oct 31, 2018

(First, apologies for the merging/reverting of this pull request. Ideally we would reopen this pull request and update it as needed, but let's use it for discussion of this proposed change)

If I understand correctly, you're running Amber under nginx, and apache isn't present anywhere in the stack. This isn't actually a configuration we've tested, and it sounds like you needed to make some nginx configuration changes to make that possible. If you can share those changes, it would give us a leg up on testing and documenting that setup.

Thoughts on the specific changes in the pull request:

  • ajaxurl variable is not available on the front-end of the site by default (unless another plugin has defined it), so we'll need to include surfacing that as part of this pull request.
  • I believe we intentionally did not provide a privileged url for logcacheview, since it's never used on the admin site

@jerclarke

@jlicht jlicht mentioned this pull request Oct 31, 2018
@carlalexander
Copy link
Contributor Author

carlalexander commented Nov 2, 2018

This isn't actually a configuration we've tested, and it sounds like you needed to make some nginx configuration changes to make that possible. If you can share those changes, it would give us a leg up on testing and documenting that setup.

I'm not sure I understand this. There's no configuration change to do on nginx. It's not how it works. You can read more about it here, but the main point is this:

Since Nginx does not have .htaccess-type capability and WordPress cannot automatically modify the server configuration for you, it cannot generate the rewrite rules for you.

This means that creating rewrite rules that only work if they are in an .htaccess file (this was the stack overflow question I linked to) won't work since WordPress can't modify the behaviour of nginx that way. The amber plugin must always point to admin-ajax.php to interact with AJAX. It cannot rely on rewrite rules like those.

I thought that ajaxurl was available in the frontend so that's my bad on that. So what we'd need is either output it manually as you describe. (Also mentioned here) I'm not sure what the preferred solution to this is, but that's one way of doing it.

As for the wp_ajax_amber_logcacheview hook, the issue was that, without this hook, Amber would not log views for logged in users. This was an issue because we still want Amber to log views if someone is logged in. So that's why this hook was added. We could add an option to revert this to the old behaviour while allowing us to continue log views with logged-in users. You also read more about it here.

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