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.

Sign up for GitHub

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

Jump to bottom

Merge REST API wrapper code #160

Merged
merged 27 commits into from
Mar 20, 2020
Merged

Merge REST API wrapper code #160

merged 27 commits into from
Mar 20, 2020

Conversation

christianwach
Copy link
Member

Overview

Merges the CiviCRM WP REST API Wrapper written by @mecachisenros into the CiviCRM WordPress plugin.

This is the first step towards dropping support for all the scripts in CiviCRM's extern directory so that CiviCRM will no longer need to try and bootstrap WordPress.

Before

There were no WordPress routes for CiviCRM scripts in extern.

After

There are now WordPress routes for CiviCRM scripts in extern.

Technical Details

See the CiviCRM WP REST API Wrapper readme for technical details. A copy of this readme is included in the PR and will be updated to reflect the new context for the code.

Comments

Although this PR means that the REST routes will be available upon merging, it should be noted that this is merely the first step towards stripping out all the directory-traversal code in CiviCRM Core which tries to locate:

  • wp-load.php
  • civicrm.settings.php

As has been discussed over the years, this code is unreliable. With the completion of all replacement REST routes, it will also become redundant.

Huge thanks to @mecachisenros for his work on this!

@kcristiano
Copy link
Member

related https://lab.civicrm.org/dev/wordpress/issues/20#civicrm-rest-api

@mecachisenros
Copy link
Contributor

Thank you @christianwach!

I've opened an issue/proposal here to introduce a hook to alter the extern URLs, which would be necessary to start testing those endpoints.

@kcristiano
Copy link
Member

@christianwach @mecachisenros I have added this PR to 5.16-RC for testing. We can run through the grid https://lab.civicrm.org/dev/rc/issues/7 to ensure that these changes do not affect existing functionality.

Do you have guidance on testing the new endpoints?

We've also added the ability to run unit tests on this repo - What type of test coverage can we get on this?

@kcristiano
Copy link
Member

@MegaphoneJon I've been testing this on the RC builds and I am not finding it hijacking any of the existing REST routes. I believe you use them in WP for a number of use cases, would you be able to take a look at this and test?

@MegaphoneJon
Copy link
Contributor

@kcristiano I don't have an active use case for this - it bit my GSoC student last summer, but that's the extent of my interest. However, I'm pretty sure @highfalutin will want to take a look at this.

@kcristiano
Copy link
Member

current Issue status and comment

@MegaphoneJon
Copy link
Contributor

Actually - an unrelated issue on an AGH extension just now made me realize that I do use the REST API on WP - it's part of my monitoring regime. It's only one use case, but I can roll this out to a couple of test sites that are under monitoring to make sure they're unaffected. Time permitting, I can also try modifying my check script to use the new endpoint.

@MegaphoneJon
Copy link
Contributor

@kcristiano @christianwach OK, just finished testing.

The good news: Applying this patch had no effect on making REST calls through the extern interface. Also good news: Applying this patch allowed me to make REST calls through the new interface.
Bad (good?) news: I found a bug during testing. Using the new REST interface to run the system.check API returns a false error that my PHP and MySQL timestamps are mismatched. This was a problem that was fixed on the extern endpoint; the relevant PR is civicrm/civicrm-core#13220.

@christianwach
Copy link
Member Author

Bad (good?) news: I found a bug during testing. Using the new REST interface to run the system.check API returns a false error that my PHP and MySQL timestamps are mismatched. This was a problem that was fixed on the extern endpoint; the relevant PR is civicrm/civicrm-core#13220.

@MegaphoneJon Nice catch, thanks Jon!

@mecachisenros You probably know better than me how best to implement this -- FWIW it's already being done prior to invoking the CiviCRM UI and prior to calling the CiviCRM API via WP-CLI. Both methods then reset the timezone afterwards here and here.

@mecachisenros
Copy link
Contributor

@MegaphoneJon @christianwach @kcristiano thank you for the tests and pointers!!

@christianwach I've submitted a PR to address it, let me know if it makes sense.

@mecachisenros mecachisenros mentioned this pull request Aug 23, 2019
Merged
mecachisenros and others added 2 commits August 23, 2019 11:53
@mecachisenros
always return $result!
bfefd78
@christianwach
Merge pull request #3 from mecachisenros/timezones
6667d91 
handle timestamps mismatched system error, CRM-12523/CRM-18062/CRM-19115
@bhahumanists
Copy link

I'm happy to test this on our live site, including the mailing URL replacements. Would that be helpful? We're running 5.13.5.

@mecachisenros
Copy link
Contributor

@bhahumanists that would be great!
Please note that it requires a minimum of PHP 7.0

mecachisenros and others added 4 commits August 29, 2019 15:26
@mecachisenros
add editor config file
7acc97c
@mecachisenros
accept api3 exception and return standardise api error data
d57b43c
@mecachisenros
assign error, don't return it from catch block
aae974c 
Always format the result!
If we call the 'rest' endpoint without json param
we are getting the error in json format
instead of the expected xml format.
@christianwach
Merge pull request #4 from mecachisenros/standard-rest-error
75475ba 
Standardise error responses for the rest endpoint
@bhahumanists
Copy link

This is installed and running without issue on our site (humanism.org.uk if you want to check anything). The new civi endpoints are exposed correctly. I'll turn on URL replacements next.

@mlutfy
Copy link
Member

mlutfy commented Sep 12, 2019 •
edited
Loading

I applied this on a site that used the REST API (and was having issues with quotes, similar to https://lab.civicrm.org/dev/core/issues/426) and this solution seems to work well (with the new endpoints).

@kcristiano
Copy link
Member

https://lab.civicrm.org/dev/wordpress/issues/20#note_23651

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@jusfreeman
Copy link

Is the only blocker for this to be merged the documentation update?

@kcristiano
Copy link
Member

related: civicrm/civicrm-core#15475

@jusfreeman I see the current blocker

Once the technical blocker is gone, we need to get the documentation PR submitted.

As merging this in will add the burden of maintenance, I want to see 'concept approved' on https://lab.civicrm.org/dev/wordpress/issues/20

All that said, I do want this merged. We've been running this on our client sites for some time now.

@kcristiano kcristiano mentioned this pull request Jan 9, 2020
Merged
mecachisenros and others added 12 commits February 4, 2020 14:07
@mecachisenros
prevent clash with CiviCRM WP REST plugin
293292a
@mecachisenros
accept instance of WP_Error
4f8dc32
@mecachisenros
add civi_wp_rest/controller/rest/permissions_check filter
084f25e
@mecachisenros
handle email (mailto) urls
5333d9b
@mecachisenros
global namesapce
22552e3
@mecachisenros
ensure namespace is an exact match
f55356e
@mecachisenros
add civi_wp_rest/plugin/replace_mailing_tracking_urls filter
cd2b095
@mecachisenros
add suport for 'civicrm_alterExternUrl' hook
cc7384a
@christianwach
Merge pull request #5 from mecachisenros/prevent-clash
369ebda 
Prevent clash with CiviCRM WP REST plugin
@christianwach
Merge pull request #6 from mecachisenros/check-permission-filter
15ccc3f 
Adds the 'civi_wp_rest/controller/rest/permissions_check' filter
@christianwach
Merge pull request #7 from mecachisenros/mailing-hooks
c756dc2 
Adds support for 'hook_civicrm_alterExternUrl' and adds a 'civi_wp_rest/plugin/replace_mailing_tracking_urls' filter
@christianwach
Merge pull request #8 from mecachisenros/minor-fixes
23906c6 
Minor fixes
@kcristiano
Copy link
Member

jenkins test this please

@kcristiano
Copy link
Member

Unrelated test fail

@kcristiano
Copy link
Member

jenkins test this please

mecachisenros and others added 2 commits March 20, 2020 12:15
@mecachisenros
log in and sync wp user before dispatching rest response
0b49768
@christianwach
Merge pull request #9 from mecachisenros/log-in-civi-user
32bf24f 
Log in and set CiviCRM user session variables user before dispatching rest response
@kcristiano
Copy link
Member

Have had this i production for quite some time. Did another r-run after the latest commits and it works as expected.

@kcristiano kcristiano merged commit 7fc81f0 into civicrm:master Mar 20, 2020
kcristiano added a commit to kcristiano/civicrm-dev-docs that referenced this pull request Mar 20, 2020
@kcristiano
Add documentation required by civicrm/civicrm-wordpress#160
fdfd0c8
@kcristiano kcristiano mentioned this pull request Mar 20, 2020
Merged
@kcristiano
Copy link
Member

Docs PR - civicrm/civicrm-dev-docs#782

kcristiano added a commit to kcristiano/civicrm-dev-docs that referenced this pull request Mar 20, 2020
@kcristiano
Add API documentation for WP REST API wrapper
b24657c 
required by civicrm/civicrm-wordpress#160
kcristiano added a commit to kcristiano/civicrm-dev-docs that referenced this pull request Mar 20, 2020
@kcristiano
Add API documentation for WP REST API wrapper
f9bad47 
required by civicrm/civicrm-wordpress#160
@christianwach christianwach deleted the evolution branch April 2, 2020 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
master needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@christianwach @kcristiano @mecachisenros @MegaphoneJon @bhahumanists @mlutfy @seamuslee001 @jusfreeman