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

Prevent session from starting during WordPress pseudo-cron procedures #17883

Closed
wants to merge 150 commits into from

Conversation

christianwach
Copy link
Member

@christianwach christianwach commented Jul 18, 2020

Overview

Fixes this issue on Lab.
There is a linked PR on CiviCRM WordPress plugin.

Before

PHP warnings are written to the logs.

After

No PHP warnings are written to the logs.

Technical Details

There are a number of routes in WordPress that do not require sessions. Overloading the sessionStart() method for WordPress means we can avoid doing so.

@civibot
Copy link

civibot bot commented Jul 18, 2020

(Standard links)

@civibot civibot bot added the master label Jul 18, 2020
@mattwire mattwire added the merge ready PR will be merged after a few days if there are no objections label Jul 18, 2020
@kcristiano
Copy link
Member

This looks good to me as well. Did an r-run as the session errors are a new issue, I think this should be against 5.28

CiviCRM and others added 25 commits July 18, 2020 20:44
These functions were deemed unused in January and deprecation warnings were added at that point.
Time to go
…ion" check

Before
------

Suppose you install CiviCRM anew.  The installer shows a bulleted list of
things to do.  One of those takes you to another, much longer list of things
to do.  And *then* there's a popup on the sidebar which says you need to
"Complete Setup".

You say to yourself, "I thought that's what I was doing!" But you click it anyway.

That takes you to a page with another list of things to do, one of which is
to "Complete Setup".  The text and link clarify that "Complete Setup"
actually means *set the name and address of the organization*.

After
-----

There are still several different pages telling you what to do after setup.

But at least the label is more precise. :)
I was able to establish that this was accidentally broken five years ago with this commit
civicrm@204c86d#diff-b00e65c9829c27da8b34e35f2e64d9b6L114

Resulting in the ipn url of
http://dmaster.local/sites/all/modules/civicrm/extern/pxIPN.php?result=00003100032465271fc8c82d94ad434d&userid=Fuzion_Dev

dying with the fatal error

 Uncaught Error: Call to undefined method CRM_Core_Payment_PaymentExpressIPN::singleton() in /Users/eileenmcnaughton/buildkit/build/dmaster/sites/all/modules/civicrm/CRM/Core/Payment/PaymentExpressIPN.php on line 391

This means we can be sure the ipn code does not work & hence can't be in use and can remove it.

Can we remove the main class too? My thinking is that there could be old instances of the payment processor in
people's installs and that if we remove or de-activate the payment processor instance it could cause
other breakage.

However, I think we could alter the processor type in the upgrade script to have
CRM_Core_Manual as the class_name in place of 'Payment_PaymentExpress'
@christianwach christianwach changed the base branch from master to 5.28 July 18, 2020 19:53
@civibot civibot bot added 5.28 and removed master labels Jul 18, 2020
@christianwach christianwach changed the base branch from 5.28 to master July 18, 2020 20:02
@civibot civibot bot added master and removed 5.28 labels Jul 18, 2020
@christianwach
Copy link
Member Author

Dammit, tried to change the base for this, have screwed up somewhere and can't repair the PR. Am closing in favour of #17890

@christianwach christianwach deleted the lab-core-1889 branch July 18, 2020 20:11
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.