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

Disable extern/soap.php. Remove implementation and tests. #25317

Merged
merged 1 commit into from
Jan 15, 2023

Conversation

totten
Copy link
Member

@totten totten commented Jan 10, 2023

Overview

There are no longer any known consumers of this interface, and it is awkward to maintain.

cc @seamuslee001

Technical Details

I left some informational stubs. There are still some other repos that reference the class CRM_Utils_SoapServer (e.g. civicrm-wordpress.git, civismtp.git), and we don't to make class-files unloadable. The WordPress integration can probably be updated in tandem, but sites may still have civismtp enabled.

(Note: Leaving stub files also addresses the Joomla loop-hole for deletion.)

@civibot
Copy link

civibot bot commented Jan 10, 2023

(Standard links)

@civibot civibot bot added the master label Jan 10, 2023
@seamuslee001
Copy link
Contributor

seems fine to me any thoughts @demeritcowboy @eileenmcnaughton @colemanw @MegaphoneJon

@eileenmcnaughton
Copy link
Contributor

I'm +1 on this. I think throwing an exception is good & that looks like a good approach

@MegaphoneJon
Copy link
Contributor

I've never really touched this, but love watching code get removed. Also I think this removes the dependency on the PHP SOAP extension?

@eileenmcnaughton
Copy link
Contributor

test this please

@totten
Copy link
Member Author

totten commented Jan 11, 2023

(@MegaphoneJon) Also I think this removes the dependency on the PHP SOAP extension?

From a core POV, yes. Separately, Civi extensions may use PHP SOAP for other reasons. A quick grep of universe suggests a few examples (com.iatspayments.civicrm, com.aghstrategies.tsys, com.groupwho.paperlesstrans). (But that's independent of extern/soap.php.)

@seamuslee001 seamuslee001 added the merge ready PR will be merged after a few days if there are no objections label Jan 12, 2023
@seamuslee001
Copy link
Contributor

Flagging as Merge ready in case anyone has any further comments

@seamuslee001
Copy link
Contributor

Probably been long enough am merging now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants