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

WordPress: avoid PHP notice if the anonymous user does not have any capabilities #19732

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

mlutfy
Copy link
Member

@mlutfy mlutfy commented Mar 4, 2021

Overview

On a site where debug mode is set (i.e. where a PHP E_NOTICE will cause fatal errors), visiting a CiviCRM form as an anonymous user will cause a confusing array_key_exists expects param 2 to be an array fatal error if the anonymous user role does not have any permissions/capabilities.

To reproduce:

  • Setup a new site
  • Enable strict PHP mode - see below
  • Visit a contribution page as an anonymous user.

Because the anonymous user does not have any capabilities, it will cause a confusing fatal error, instead of a "permission denied".

Example to enable fatal triggers, if xdebug is enabled, add to civicrm.settings.php:

<?php
ini_set('xdebug.halt_level', E_WARNING|E_NOTICE|E_USER_WARNING|E_USER_NOTICE);

I haven't really tested. I was debugging a site using Themosis (not an endorsement, I was helping out another shop).

Before

before-0

fatal

After

before-1

Comments

cc @christianwach @kcristiano

@civibot civibot bot added the master label Mar 4, 2021
@civibot
Copy link

civibot bot commented Mar 4, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

prize for best illustrations

@totten
Copy link
Member

totten commented Mar 5, 2021

Successful reviewer gets to say, "Make it so!"

@christianwach
Copy link
Member

@mlutfy This should not occur with a properly installed CiviCRM. On activation, the anonymous user is created and it is granted the default set of capabilities. The $roleObj->get_role('anonymous_user')->capabilities array should therefore always be populated.

The error you see suggests that the install procedure has not run correctly and therefore the fatal error is appropriate to let you know that you need to review your setup.

@mlutfy
Copy link
Member Author

mlutfy commented Mar 5, 2021

@christianwach For what it's worth, the problem happened on a wordpress-multisite installation. CiviCRM was installed normally, then enabled on the other network sites.

The code change is just a bit of tidying up. Should not have any impact otherwise.

Even the fatal itself, is happening because this shop was using a theme with strict debugging settings. They keep stumbling on weird bugs because of this, but I figure it's better to tidyup anyway.

@christianwach
Copy link
Member

CiviCRM was installed normally, then enabled on the other network sites.

@mlutfy This should still trigger the activation routine on each sub-site unless CiviCRM is network-activated.

The danger with CiviCRM activated per-site in multisite is that the default is to overwrite the existing database during activation unless separate database is specified or there is a common civicrm.settings.php file like there would be in a CiviCRM multi-domain setup.

The anonymous role and its capabilities should still be created regardless, so I'm curious why this isn't happening in your example and what we can do about that.

Even the fatal itself, is happening because this shop was using a theme with strict debugging settings.

Fair enough, on reflection it seems unreasonable to prevent them from using the UI until the problem is resolved.

@mlutfy
Copy link
Member Author

mlutfy commented Mar 8, 2021

"The anonymous role and its capabilities should still be created regardless, so I'm curious why this isn't happening in your example and what we can do about that."

To be honest this feels like a bit of a rabbit hole. Techy people unfamiliar with CiviCRM tend to do all sorts of non-standard things (from CiviCRM's point of view).

Any objections to merging the PR?

@christianwach
Copy link
Member

Any objections to merging the PR?

Not here - go for it 👍

@mlutfy mlutfy merged commit c6230f2 into civicrm:master Mar 9, 2021
@mlutfy
Copy link
Member Author

mlutfy commented Mar 9, 2021

Thanks @christianwach !

@eileenmcnaughton eileenmcnaughton deleted the fixWpAnon branch March 9, 2021 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants