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

Admin Bar prevents Logged-In User caching from working #690

Closed
raamdev opened this issue Feb 26, 2016 · 7 comments
Closed

Admin Bar prevents Logged-In User caching from working #690

raamdev opened this issue Feb 26, 2016 · 7 comments
Assignees
Milestone

Comments

@raamdev
Copy link
Contributor

@raamdev raamdev commented Feb 26, 2016

When Logged-In User caching is enabled in Comet Cache Pro, all pages on the site for a logged-in user are not cached by default, because by default WordPress has the WordPress Admin (aka the Toolbar) bar enabled for user for logged-in users, and the Admin Bar that adds a wpnonce to the page source on the front-end, which then disables Comet Cache caching.

2016-02-26_09-07-44
2016-02-26_09-09-44
2016-02-26_09-10-41

If Comet Cache disables the Admin Bar on the front-end of the site (when Logged-In User caching is enabled), this issue goes away.

So, when Logged-In User caching is enabled, Comet Cache should disable the WordPress Admin Bar for logged-in users (on the front-end of the site). There should also be an option in the Logged-In Users panel that allows site owners to override this (i.e., turn back on the Admin Bar when Logged-In User caching is enabled, in case they want to do that because maybe they're going to allow Nonce caching using COMET_CACHE_CACHE_NONCE_VALUES_WHEN_LOGGED_IN; see this article).

@jaswrks
Copy link

@jaswrks jaswrks commented Apr 8, 2016

@renzms The way to disable the Admin Bar in WordPress is by calling show_admin_bar(false);. It looks like this most of the work needed to complete this issue has already been implemented. However, as it stands now it is not configurable, so that would be something that you could add to the UI if you'd like.

See: https://github.com/websharks/comet-cache-pro/blob/160227/src/includes/classes/Plugin.php#L536

The current behavior is controlled only by a filter and not by a configurable plugin option that is exposed by the UI for the Comet Cache plugin. That's what we need next here.

@renzms
Copy link

@renzms renzms commented Apr 18, 2016

(edited and moved to PR here)

renzms added a commit to wpsharks/comet-cache-pro that referenced this issue Apr 23, 2016
renzms added a commit to wpsharks/comet-cache-pro that referenced this issue Apr 29, 2016
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented May 10, 2016

Noting that the existing code that disables the admin bar only checks if $this->options['when_logged_in'] === '1', even though 1 is no longer an option (that option was hidden a while back when decided to stop offering the option to enable user caching but not maintain a separate cache; see these lines in the option panel).

As a result, the Admin Bar is not disabled right now when you enable Logged-In User Caching:

if ($this->options['when_logged_in'] === '1' && $this->applyWpFilters(GLOBAL_NS.'_when_logged_in_no_admin_bar', true)) {
            show_admin_bar(false); // Prevent admin bar from being cached.
        }

The available options now are 0 (disable logged-in user caching) and postload (enable logged-in user caching and separate the cache for each user). (Note: We do still need to consider option 1—enable logged-in user caching but don't maintain a separate cache—because we make it possible for users who were previously using that option to continue using and seeing that option.)

So unless I'm mistaken, we need to update the existing code to include a check for postload:

if (($this->options['when_logged_in'] === '1' || $this->options['when_logged_in'] === 'postload') && $this->applyWpFilters(GLOBAL_NS.'_when_logged_in_no_admin_bar', true)) {
            show_admin_bar(false); // Prevent admin bar from being cached.
        }

@jaswsinc If you could verify the above to confirm that I'm not missing anything, that would be great.

@raamdev raamdev modified the milestones: Next Release, Future Release May 11, 2016
@jaswrks
Copy link

@jaswrks jaswrks commented May 12, 2016

Yes, I can confirm that is a bug. I'd change it to look for any non-empty value like this:

if ($this->options['when_logged_in'] && $this->applyWpFilters(GLOBAL_NS.'_when_logged_in_no_admin_bar', true)) {
            show_admin_bar(false); // Prevent admin bar from being cached.
        }
renzms added a commit to wpsharks/comet-cache-pro that referenced this issue May 13, 2016
raamdev added a commit to wpsharks/comet-cache-pro that referenced this issue May 13, 2016
raamdev added a commit to wpsharks/comet-cache-pro that referenced this issue May 13, 2016
Disabled is the recommended setting, so we should default to disabled.

See wpsharks/comet-cache#690
raamdev added a commit to wpsharks/comet-cache-pro that referenced this issue May 13, 2016
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented May 13, 2016

Next Release Changelog:

  • Enhancement (Pro): It's now possible to disable the WordPress Admin Toolbar when Logged-In User Caching is enabled with a new option in Comet Cache → Plugin Options → Logged-In Users → Disable the Admin Toolbar for Logged-In Users & Comment Authors? Props @renzms and @KTS915. See Issue #690.
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented May 16, 2016

Noting that the default behavior of this new option was changed during the RC period for this release. The default is now to leave the Admin Bar untouched, i.e., not disable the Admin Bar by default when Logged-In Users caching is enabled. See wpsharks/comet-cache-pro@e099423.

@raamdev writes in #769 (comment):

Here's what I'm going to do to get the next release out: I'm going to change the default for this new setting so that it doesn't touch the Admin Toolbar by default. If a site owner wants to disable the Admin Bar using Comet Cache, they can flip the setting in the Logged-In Users panel, but for now I'm going to make this disabled by default so that if a site owner is already using a plugin to disable the Admin Bar, Comet Cache won't introduce a conflict (as @KTS915 experienced with Clean Login).

Once we're through this release cycle, we can revisit this issue and decide if and how we're going to change the Nonce caching behavior.

Updated screenshot of the option panel (includes new copy and a bit more description):

2016-05-16_19-18-39

@raamdev raamdev closed this May 16, 2016
@raamdev
Copy link
Contributor Author

@raamdev raamdev commented May 21, 2016

Comet Cache v160521 has been released and includes changes from this GitHub Issue. See the v160521 announcement for further details.


This issue will now be locked to further updates. If you have something to add related to this GitHub Issue, please open a new GitHub Issue and reference this one (#690).

@wpsharks wpsharks locked and limited conversation to collaborators May 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants