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

dev/core#3914 Add getRoleNames() method for WordPress #24751

Merged
merged 2 commits into from
Oct 25, 2022

Conversation

bastienho
Copy link
Contributor

Overview

Fixes the Missing getRoleNames() method in WordPress System Utility issue.

Some extensions use the CRM_Core_Config::singleton()->userSystem->getRoleNames() method, which is not implemented for WordPress

Before

On a WordPress install, CRM_Core_Config::singleton()->userSystem->getRoleNames() return NULL;

After

CRM_Core_Config::singleton()->userSystem->getRoleNames() returns

Array
(
    [administrator] => Administrator
    [editor] => Editor
    [author] => Author
    [contributor] => Contributor
    [subscriber] => Subscriber
    [anonymous_user] => Anonymous User
)

Technical Details

Uses wp_roles()

@civibot
Copy link

civibot bot commented Oct 14, 2022

(Standard links)

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@civibot civibot bot added the master label Oct 14, 2022
@bastienho bastienho changed the title Add getRoleNames() method for WordPress dev/core#3914 Add getRoleNames() method for WordPress Oct 14, 2022
@seamuslee001
Copy link
Contributor

Jenkins add to white list

@seamuslee001
Copy link
Contributor

this seems ok to me thoughts @kcristiano @haystack?

@seamuslee001
Copy link
Contributor

Jenkins add to whitelist

@@ -1283,6 +1283,15 @@ public function sessionStart() {
session_start();
}
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

This seems intuitively sensible.

@bastienho I think the test bot is complaining because the blank line (1286 above) has some invisible space characters. If you can delete the space-characters, then it should accept it.

@kcristiano
Copy link
Member

This looks sensible. I've applied the patch to my RC testimg site to test. @bastienho do you have an example of an extension that I can specifically test?

@bastienho
Copy link
Contributor Author

bastienho commented Oct 21, 2022 via email

@kcristiano
Copy link
Member

That's odd on CiviCRM master enabling that extension fails

Enabling extension "hierarchicalacl"
Error: API Call Failed: Array
(
    [entity] => Extension
    [action] => install
    [params] => Array
        (
            [keys] => Array
                (
                    [0] => hierarchicalacl
                )

            [debug] => 1
            [version] => 3
        )

    [result] => Array
        (
            [is_error] => 1
            [error_message] => Unknown extension: jsoneditor
            [xdebug] => Array
                (
                )

        )

)

Just standard buildkit extensions installed

key name status version
authx authx installed 5.56.alpha1
civicrm_admin_ui civicrm_admin_ui uninstalled 5.56.alpha1
civigrant civigrant installed 5.56.alpha1
civiimport civiimport uninstalled 5.56.alpha1
ckeditor4 ckeditor4 installed 5.56.alpha1
contributioncancelactions contributioncancelactions uninstalled 5.56.alpha1
elavon elavon uninstalled 5.56.alpha1
eu.tttp.civisualize civisualize installed 6.1
eventcart eventcart installed 5.56.alpha1
ewaysingle ewaysingle uninstalled 5.56.alpha1
financialacls financialacls installed 5.56.alpha1
greenwich greenwich installed 5.56.alpha1
hierarchicalacl hierarchicalacl uninstalled 1.1.0
legacycustomsearches legacycustomsearches installed 5.56.alpha1
message_admin message_admin uninstalled 5.56.alpha1
oauth-client oauth_client uninstalled 5.56.alpha1
org.civicoop.civirules civirules installed 2.44-dev
org.civicrm.afform afform installed 5.56.alpha1
org.civicrm.afform-html afform_html uninstalled 5.56.alpha1
org.civicrm.afform-mock afform_mock uninstalled 5.56.alpha1
org.civicrm.afform_admin afform_admin installed 5.56.alpha1
org.civicrm.angularex angularex uninstalled 1
org.civicrm.angularprofiles angularprofiles installed 1.2.2
org.civicrm.contactlayout contactlayout installed 2.1.1
org.civicrm.demoqueue demoqueue uninstalled 1
org.civicrm.flexmailer flexmailer installed 5.56.alpha1
org.civicrm.module.cividiscount cividiscount installed 3.8.6
org.civicrm.search_kit search_kit installed 5.56.alpha1
org.civicrm.volunteer volunteer installed 2.4.3
org.example.scssmethodtest scssmethodtest uninstalled 1
payflowpro payflowpro uninstalled 5.56.alpha1
recaptcha recaptcha installed 5.56.alpha1
sequentialcreditnotes sequentialcreditnotes installed 5.56.alpha1
sequentialcreditnotes sequentialcreditnotes installed 5.56.alpha1

@christianwach
Copy link
Member

Seems like a sensible addition to me. I expect it's worth checking that "ACL Group/Role" multi-select functions as expected in whatever form it is that CRM_Report_Form_Instance::buildForm() renders - and that the functionality it implements works too.

BTW @seamuslee001 I know it's confusing, but someone grabbed my usual handle here before I did. 🤷

@kcristiano kcristiano added merge ready PR will be merged after a few days if there are no objections and removed merge ready PR will be merged after a few days if there are no objections labels Oct 21, 2022
@kcristiano
Copy link
Member

kcristiano commented Oct 21, 2022

I've been able to do a number of r-run and see no reason not to merge (once the white space is cleaned up).

@bastienho
Copy link
Contributor Author

Sorry guys, the white space is cleaned-up

@seamuslee001 seamuslee001 merged commit 206646a into civicrm:master Oct 25, 2022
@seamuslee001
Copy link
Contributor

thanks @bastienho can you please also create a PR to add yourself to the Contributor Key file https://github.com/civicrm/civicrm-core/blob/master/contributor-key.yml

@bastienho bastienho deleted the patch-1 branch October 26, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants